Re: [PATCH 3/7] hw/ide/core: Replace magic '512' value by BDRV_SECTOR_SIZE

[Philippe Mathieu-Daudé]

On 9/23/20 4:53 PM, John Snow wrote:
> On 8/17/20 7:17 AM, Kevin Wolf wrote:
>> Am 14.08.2020 um 10:28 hat Philippe Mathieu-Daudé geschrieben:
>>> Use self-explicit definitions instead of magic '512' value.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> BDRV_SECTOR_SIZE is the arbitrary unit in which some block layer
>> functions and variables work (such as bs->total_sectors). It happens to
>> be 512.
>>
>> IDE disks have a sector size, too. Actually, two of them, a physical and
>> a logical one. The more important one is the logical one. We happen to
>> emulate only IDE devices for which the logical block size is 512 bytes
>> (ide_dev_initfn() errors out otherwise).
>>
>> But just because two constants both happen to be 512 in practice, they
>> are not the same thing.
>>
>> So if we want to replace all magic 512 values, we should probably
>> introduce a new IDE_SECTOR_SIZE and then decide case by case whether
>> IDE_SECTOR_SIZE or BDRV_SECTOR_SIZE is meant. I think most (if not all)
>> of the places you converted in this patch need to be IDE_SECTOR_SIZE.
>>
>> Kevin
>>
> 
> I didn't audit the other patches, but be mindful of the distinction that
> Kevin is pointing out.
> 
> Luckily, I think we're low risk for deciding to change the
> BDRV_SECTOR_SIZE default any time soon, so it probably won't matter in
> the near future ...

TBO my only concern was code readability while reviewing
(improve code readability).

I'll address Kevin's review comment at some point, but this is
a low priority.

Thanks both for having a look,

Phil.

> 
> --js
> 
>