Re: [Qemu-devel] [PATCH v3 1/5] block: add bdrv functions for geometry and blocksize

[Markus Armbruster]

Ekaterina Tumanova <address@hidden> writes:

> Add driver functions for geometry and blocksize detection
>
> Signed-off-by: Ekaterina Tumanova <address@hidden>
> ---
>  block.c                   | 35 +++++++++++++++++++++++++++++++++++
>  include/block/block.h     | 13 +++++++++++++
>  include/block/block_int.h |  5 +++++
>  3 files changed, 53 insertions(+)
>
> diff --git a/block.c b/block.c
> index a612594..7b0b804 100644
> --- a/block.c
> +++ b/block.c
> @@ -548,6 +548,41 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error 
> **errp)
>      }
>  }
>  
> +/**
> + * Try to get @bs's logical and physical block size.
> + * On success, store them in bsz struct.

@bsz

> + * On failure, set default blocksize.
> + */

Suggest not to talk about failure here.  Like this:

/**
 * Get @bs's logical and physical block size, store them in @bsz.
 */

> +void bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    assert(drv != NULL);

Dies when @bs is empty (no medium).  Either return default block sizes
instead, or amend the function contract with "@bs must not be empty."

Should've caught that in v2, sorry.

> +    if (drv->bdrv_probe_blocksizes &&
> +        !drv->bdrv_probe_blocksizes(bs, bsz)) {
> +            return;
> +    }
> +    bsz->log = BDRV_SECTOR_SIZE;
> +    bsz->phys = BDRV_SECTOR_SIZE;
> +}
> +
> +/**
> + * Try to get @bs's geometry (cyls, heads, sectos)
> + * On success, store them in geo struct and return 0.

... store it in @geo and ...

> + * On failure return -errno.
> + */
> +int bdrv_probe_geometry(BlockDriverState *bs, hdGeometry *geo)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    assert(drv != NULL);

Dies when @bs is empty (no medium).  Either return -ENOMEDIUM instead,
or amend the function contract with "@bs must not be empty."  The former
seems simpler to me.

> +    if (drv->bdrv_probe_geometry) {
> +        return drv->bdrv_probe_geometry(bs, geo);
> +    }
> +
> +    return -1;

This is not a negative errno!  I'd return -ENOTSUP.

> +}
> +
>  /*
>   * 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..17184b6 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -60,6 +60,17 @@ typedef enum {
>      BDRV_REQ_MAY_UNMAP    = 0x4,
>  } BdrvRequestFlags;
>  
> +typedef struct BlockSizes {
> +    uint32_t phys;
> +    uint32_t log;
> +} BlockSizes;
> +
> +typedef struct hdGeometry {
> +    uint32_t heads;
> +    uint32_t sectors;
> +    uint32_t cylinders;
> +} hdGeometry;
> +
>  #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 +549,8 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
>   * the old #AioContext is not executing.
>   */
>  void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context);
> +void bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz);
> +int bdrv_probe_geometry(BlockDriverState *bs, hdGeometry *geo);
>  
>  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..16e53e9 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -271,6 +271,11 @@ struct BlockDriver {
>      void (*bdrv_io_unplug)(BlockDriverState *bs);
>      void (*bdrv_flush_io_queue)(BlockDriverState *bs);
>  
> +    /* try to get physical and logical blocksizes */
> +    int (*bdrv_probe_blocksizes)(BlockDriverState *bs, BlockSizes *bsz);

I recommend a function contract, not just a paraphrasing of the function
name.  The one you put in block.c needs just a small touchup to fit
here nicely:

/**
 * Try to get @bs's logical and physical block size.
 * On success, store them in @bsz and return zero.
 * On failure, return negative errno.
 */

> +    /* tey to get hard drive geometry (cyls, head, sectors) */

s/tey/try/

> +    int (*bdrv_probe_geometry)(BlockDriverState *bs, hdGeometry *geo);
> +

/**
 * Try to get @bs's geometry (cyls, heads, sectos)
 * On success, store them in @geo and return 0.
 * On failure return -errno.
 */

>      QLIST_ENTRY(BlockDriver) list;
>  };