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

[John Snow <jsnow@redhat.com>]

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 ...

--js