John Snow <js...@redhat.com> 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.
