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é <f4...@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 > >
