Re: [Qemu-devel] [PATCH v3 02/10] block: New bdrv_nb_sectors()

[Benoît Canet]

The Friday 30 May 2014 à 20:13:43 (+0200), Markus Armbruster wrote :
> A call to retrieve the image size converts between bytes and sectors
> several times:
> 
> * BlockDriver method bdrv_getlength() returns bytes.
> 
> * refresh_total_sectors() converts to sectors, rounding up, and stores
>   in total_sectors.
> 
> * bdrv_getlength() converts total_sectors back to bytes (now rounded
>   up to a multiple of the sector size).
> 
> * Callers wanting sectors rather bytes convert it right back.
>   Example: bdrv_get_geometry().
> 
> bdrv_nb_sectors() provides a way to omit the last two conversions.
> It's exactly bdrv_getlength() with the conversion to bytes omitted.
> It's functionally like bdrv_get_geometry() without its odd error
> handling.
> 
> Reimplement bdrv_getlength() and bdrv_get_geometry() on top of
> bdrv_nb_sectors().
> 
> The next patches will convert some users of bdrv_getlength() to
> bdrv_nb_sectors().
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  block.c               | 29 +++++++++++++++++++----------
>  include/block/block.h |  1 +
>  2 files changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/block.c b/block.c
> index a517d72..14dc5fe 100644
> --- a/block.c
> +++ b/block.c
> @@ -693,6 +693,7 @@ static int find_image_format(BlockDriverState *bs, const 
> char *filename,
>  
>  /**
>   * Set the current 'total_sectors' value
> + * Return 0 on success, -errno on error.
>   */
>  static int refresh_total_sectors(BlockDriverState *bs, int64_t hint)
>  {
> @@ -3551,11 +3552,12 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState 
> *bs)
>  }
>  
>  /**
> - * Length of a file in bytes. Return < 0 if error or unknown.
> + * Return number of sectors on success, -errno on error.
>   */
> -int64_t bdrv_getlength(BlockDriverState *bs)
> +int64_t bdrv_nb_sectors(BlockDriverState *bs)
>  {
>      BlockDriver *drv = bs->drv;
> +
>      if (!drv)
>          return -ENOMEDIUM;
>  
> @@ -3565,19 +3567,26 @@ int64_t bdrv_getlength(BlockDriverState *bs)
>              return ret;
>          }
>      }
> -    return bs->total_sectors * BDRV_SECTOR_SIZE;
> +    return bs->total_sectors;
> +}
> +
> +/**
> + * Return length in bytes on success, -errno on error.
> + * The length is always a multiple of BDRV_SECTOR_SIZE.
> + */
> +int64_t bdrv_getlength(BlockDriverState *bs)
> +{
> +    int64_t ret = bdrv_nb_sectors(bs);
> +
> +    return ret < 0 ? ret : ret * BDRV_SECTOR_SIZE;
>  }
>  
>  /* return 0 as number of sectors if no device present or error */
>  void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr)
>  {
> -    int64_t length;
> -    length = bdrv_getlength(bs);
> -    if (length < 0)
> -        length = 0;
> -    else
> -        length = length >> BDRV_SECTOR_BITS;
> -    *nb_sectors_ptr = length;
> +    int64_t nb_sectors = bdrv_nb_sectors(bs);
> +
> +    *nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors;
>  }
>  
>  void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error,
> diff --git a/include/block/block.h b/include/block/block.h
> index faee3aa..4a1e875 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -279,6 +279,7 @@ BlockDriverState 
> *bdrv_find_backing_image(BlockDriverState *bs,
>      const char *backing_file);
>  int bdrv_get_backing_file_depth(BlockDriverState *bs);
>  int bdrv_truncate(BlockDriverState *bs, int64_t offset);
> +int64_t bdrv_nb_sectors(BlockDriverState *bs);
>  int64_t bdrv_getlength(BlockDriverState *bs);
>  int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
>  void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
> -- 
> 1.9.3
> 
> 
Reviewed-by: Benoit Canet <address@hidden>