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

[Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>]

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 <tumanova@linux.vnet.ibm.com> writes:
>
>> Add driver functions for geometry and blocksize detection
>>
>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>> ---
>>   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.
I thought probe might be more suitable, since we do not "get" the value
for sure. maybe "detect"?

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

> Such a struct should ideally be used in other places where we store both
> sizes.
>
> A brief function contract comment wouldn't hurt.  Something like
>
>      /*
>       * Try to get @bs's logical and physical block size.
>       * Block sizes are always a multiple of BDRV_SECTOR_SIZE.
>       * On success, store them in ... and return 0.
>       * On failure, return -errno.
>       */
>
will do

>> +
>> +struct ProbeGeometry bdrv_probe_geometry(BlockDriverState *bs)
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +    struct ProbeGeometry err_geo = { .rc = -1 };
>> +
>> +    assert(drv != NULL);
>> +    if (drv->bdrv_probe_geometry) {
>> +        return drv->bdrv_probe_geometry(bs);
>> +    }
>> +
>> +    return err_geo;
>> +}
>> +
>>   /*
>>    * Create a uniquely-named empty temporary file.
>>    * Return 0 upon success, otherwise a negative errno value.
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 5450610..3287dbc 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -60,6 +60,24 @@ typedef enum {
>>       BDRV_REQ_MAY_UNMAP    = 0x4,
>>   } BdrvRequestFlags;
>>
>> +struct ProbeBlockSize {
>> +    int rc;
>> +    struct BlockSize {
>> +        uint16_t phys;
>> +        uint16_t log;
>> +    } size;
>> +};
>
> Use of uint16_t for block sizes is silly, but not your fault, you just
> follow existing usage.
>
>> +
>> +struct ProbeGeometry {
>> +    int rc;
>> +    struct HDGeometry {
>> +        uint32_t heads;
>> +        uint32_t sectors;
>> +        uint32_t cylinders;
>> +        uint32_t start;
>> +    } geo;
>> +};
>> +
>>   #define BDRV_O_RDWR        0x0002
>>   #define BDRV_O_SNAPSHOT    0x0008 /* open the file read only and save writes in a snapshot */
>>   #define BDRV_O_TEMPORARY   0x0010 /* delete the file after use */
>> @@ -538,6 +556,8 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
>>    * the old #AioContext is not executing.
>>    */
>>   void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context);
>> +struct ProbeBlockSize bdrv_probe_blocksizes(BlockDriverState *bs);
>> +struct ProbeGeometry bdrv_probe_geometry(BlockDriverState *bs);
>>
>>   void bdrv_io_plug(BlockDriverState *bs);
>>   void bdrv_io_unplug(BlockDriverState *bs);
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index a1c17b9..830e564 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -271,6 +271,9 @@ struct BlockDriver {
>>       void (*bdrv_io_unplug)(BlockDriverState *bs);
>>       void (*bdrv_flush_io_queue)(BlockDriverState *bs);
>>
>> +    struct ProbeBlockSize (*bdrv_probe_blocksizes)(BlockDriverState *bs);
>> +    struct ProbeGeometry (*bdrv_probe_geometry)(BlockDriverState *bs);
>> +
>>       QLIST_ENTRY(BlockDriver) list;
>>   };
>
> Callbacks need contracts even more than functions do.  I know this file
> is full of bad examples.  Let's not add more :)
>

Thanks!
Kate.