mbox series

[0/7] block: Use definitions instead of magic values

Message ID 20200814082841.27000-1-f4bug@amsat.org
Headers show
Series block: Use definitions instead of magic values | expand

Message

Philippe Mathieu-Daudé Aug. 14, 2020, 8:28 a.m. UTC 
Trivial block patches:
- Fix a typo
- Replace '1 << 30' by '1 * GiB' in null-co
- Replace 512 by BDRV_SECTOR_SIZE when appropriate.

Philippe Mathieu-Daudé (7):
  block/null: Make more explicit the driver default size is 1GiB
  hw/ide/core: Trivial typo fix
  hw/ide/core: Replace magic '512' value by BDRV_SECTOR_SIZE
  hw/ide/ahci: Replace magic '512' value by BDRV_SECTOR_SIZE
  hw/ide/atapi: Replace magic '512' value by BDRV_SECTOR_SIZE
  hw/ide/pci: Replace magic '512' value by BDRV_SECTOR_SIZE
  hw/scsi/scsi-disk: Replace magic '512' value by BDRV_SECTOR_SIZE

 block/null.c        |  4 +++-
 hw/ide/ahci.c       |  5 +++--
 hw/ide/atapi.c      |  8 ++++----
 hw/ide/core.c       | 25 +++++++++++++------------
 hw/ide/pci.c        |  2 +-
 hw/scsi/scsi-disk.c | 44 +++++++++++++++++++++++---------------------
 6 files changed, 47 insertions(+), 41 deletions(-)

Comments

John Snow Sept. 23, 2020, 2:53 p.m. UTC | #1 
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
Philippe Mathieu-Daudé Sept. 23, 2020, 4:57 p.m. UTC | #2 
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

> 

>