[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/5] block: New bdrv_nb_sectors()
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 2/5] block: New bdrv_nb_sectors() |
Date: |
Mon, 12 May 2014 14:36:09 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) |
Kevin Wolf <address@hidden> writes:
> Am 09.05.2014 um 11:48 hat Markus Armbruster geschrieben:
>> A call to retrieve the image size converts between bytes and sectors
>> several times:
>>
>> * BlockDriver method bdrv_getlength() returns bytes.
>>
>> * refresh_total_sectors() converts to sectors, rounding up, and stores
>> in total_sectors.
>>
>> * bdrv_getlength() converts total_sectors back to bytes (now rounded
>> up to a multiple of the sector size).
>>
>> * Callers wanting sectors rather bytes convert it right back.
>> Example: bdrv_get_geometry().
>>
>> bdrv_nb_sectors() provides a way to omit the last two conversions.
>> It's exactly bdrv_getlength() with the conversion to bytes omitted.
>> It's functionally like bdrv_get_geometry() without its odd error
>> handling.
>>
>> Reimplement bdrv_getlength() and bdrv_get_geometry() on top of
>> bdrv_nb_sectors().
>>
>> The next patches will convert some users of bdrv_getlength() to
>> bdrv_nb_sectors().
>>
>> Signed-off-by: Markus Armbruster <address@hidden>
>
> Is this really the right direction to move?
>
> Generally I think we should be trying to move away from those arbitrary
> units of 512 bytes (that are called sectors for some reason, but aren't
> really related to either guest or host sector sizes), and towards
> interfaces that use byte granularity.
My patches neither move towards nor away from a byte-based interface.
> So I would rather see bs->total_sectors becoming bs->total_bytes than
> adding a new sector-based function.
Doing away with BDRV_SECTOR_SIZE & friends is beyond my reach.
My initial hope was to replace bdrv_get_geometry() by bdrv_nb_sectors(),
but I ran out of steam reviewing whether and how the callers should
handle errors. So yes, this adds another sector-based function. But I
believe it makes the code a bit simpler, and won't make an eventual
conversion to byte-based any harder.