qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2] ide: Add resize callback to ide/core


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v2] ide: Add resize callback to ide/core
Date: Thu, 14 Aug 2014 22:15:55 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)

John Snow <address@hidden> writes:

> On 08/14/2014 03:12 AM, Markus Armbruster wrote:
>>
>> I'd prefer if (s->drive_kind == IDE_CFATA) ... else ..., because it
>> avoids the double negative.
>
> OK. This is how cmd_identify delegates. For matters of style I usually
> try to defer to nearby code.
>
>>
>> Your code duplicates the parts of the functions initializing
>> s->identify_data dealing with nb_sectors.  These are:
>>
>> * ide_identify() for IDE_HD
>>
>>    Writes nb_sectors to slots 60..61 and 100..103.  Matches your code.
>>
>> * ide_atapi_identify() for IDE_CD
>>
>>    Doesn't use it at all.  Your code clobbers slots 60..61 and 100.103.
>>    Oops.
>>
>
> ide_resize_cb does not get called for ATAPI drives, so I think this is
> not relevant. I tried to note this in a comment. See ide_init_drive:
>
>     if (kind == IDE_CD) {
>         bdrv_set_dev_ops(bs, &ide_cd_block_ops, s);
>         ...
>     } else {
>         ...
>         bdrv_set_dev_ops(bs, &ide_hd_block_ops, s);
>     }

You're right.

You could assert it in ide_resize_cb(), like this:

    if (s->drive_kind == IDE_HD) {
        ....
    } else {
        assert(s->drive_kind == IDE_CFATA);
        ...
    }

>> * ide_cfata_identify() for IDE_CFATA
>>
>>    Writes nb_sectors to slots 7..8 and and 60..61.  Your code only writes
>>    the former.
>
> ...Sorry. I appreciate you taking your time to review these patches. I
> will submit a V3.

No problem, we're all human.

>> I guess code duplication is tolerable, because the format of
>> s->identify_data is unlikely to change.  Comments linking the copies
>> together wouldn't hurt, though.
>>
>> If we don't want the duplication, we need to factor the length code out
>> of the identify functions, so we can use it in both places.
>>
>
> The length and complexity didn't feel like it needed to be factored
> out into two new functions, and I liked the idea of keeping the writes
> to the buffer sequential inside the initialization functions, because
> it made it easier for me to read along with the spec that
> way. Factoring it out seemed more of a fruitless hassle than anything.

You're probably right.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]