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

[Ekaterina Tumanova]

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