Re: [Qemu-devel] [PATCH v2 1/6] geometry: add bdrv functions for geometry and blocksize

[Markus Armbruster]

Ekaterina Tumanova <address@hidden> writes:

> On 11/27/2014 05:55 PM, Markus Armbruster wrote:
>> I'm sorry for the delay.  I got the flu and have difficulties thinking
>> straight for longer than a few minutes.
>>
>> Ekaterina Tumanova <address@hidden> writes:
>>
>>> Add driver functions for geometry and blocksize detection
>>>
>>> Signed-off-by: Ekaterina Tumanova <address@hidden>
>>> ---
>>>   block.c                   | 26 ++++++++++++++++++++++++++
>>>   include/block/block.h     | 20 ++++++++++++++++++++
>>>   include/block/block_int.h |  3 +++
>>>   3 files changed, 49 insertions(+)
>>>
>>> diff --git a/block.c b/block.c
>>> index a612594..5df35cf 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -548,6 +548,32 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error 
>>> **errp)
>>>       }
>>>   }
>>>
>>> +struct ProbeBlockSize bdrv_probe_blocksizes(BlockDriverState *bs)
>>> +{
>>> +    BlockDriver *drv = bs->drv;
>>> +    struct ProbeBlockSize err_geo = { .rc = -1 };
>>> +
>>> +    assert(drv != NULL);
>>> +    if (drv->bdrv_probe_blocksizes) {
>>> +        return drv->bdrv_probe_blocksizes(bs);
>>> +    }
>>> +
>>> +    return err_geo;
>>> +}
>>
>> I'm not sure about "probe".  Naming is hard.  "get"?
>>
> There was already "bdrv_get_geometry", and I wanted the _geometry and
> _blocksize functions to use the same naming convention.

Fair enough.

bdrv_get_geometry() is a silly wrapper around bdrv_nb_sectors() that
maps failure to zero sectors.  I hope to kill it some time.  Doesn't
help you now.

> I thought probe might be more suitable, since we do not "get" the value
> for sure. maybe "detect"?

Feel free to stick to "probe".

>> Squashing status and actual payload into a single struct to use as
>> return type isn't wrong, but unusual.  When the payload can't represent
>> failure conveniently, we usually return status, and write the payload to
>> a buffer provided by the caller, like this:
>>
>>      int bdrv_get_blocksizes(BlockDriverState *bs,
>>                              uint16_t *physical_blk_sz,
>>                              uint16_t *logical_blk_sz)
>>
>> Or, with a struct to hold both sizes:
>>
>>      int bdrv_get_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
>>
> Do you insist on changing that? Returning a struct via stack seemed
> useful to me, since there was less probability of caller allocating
> a buffer of incorrect size or smth like that.

You'd have to do crazy stuff to defeat the static type checker.

Please stick to the common technique.

[...]