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

[Markus Armbruster]

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"?

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)

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

> +
> +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 :)