Re: [Qemu-devel] [PATCH v2 16/17] block: Split bdrv_merge_limits() from bdrv_refresh_limits()

[Kevin Wolf]

Am 14.06.2016 um 23:30 hat Eric Blake geschrieben:
> The raw block driver was blindly copying all limits from bs->file,
> even though: 1. the main bdrv_refresh_limits() already does this
> for many of gthe limits, and 2. blindly copying from the children
> can weaken any stricter limits that were already inherited from
> the backing dhain during the main bdrv_refresh_limits().  Also,
> the next patch is about to move .request_alignment into
> BlockLimits, and that is a limit that should NOT be copied from
> other layers in the BDS chain.
> 
> Solve the issue by factoring out a new bdrv_merge_limits(),
> and using that function to properly merge limits when comparing
> two BlockDriverState objects.
> 
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> v2: new patch
> ---
>  include/block/block.h     |  1 +
>  include/block/block_int.h |  4 ++--
>  include/qemu/typedefs.h   |  1 +
>  block/io.c                | 31 +++++++++++++------------------
>  block/raw_bsd.c           |  4 ++--
>  5 files changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 733a8ec..c1d4648 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -262,6 +262,7 @@ 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);
> +void bdrv_merge_limits(BlockLimits *dst, const BlockLimits *src);
>  void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
>  int bdrv_commit(BlockDriverState *bs);
>  int bdrv_change_backing_file(BlockDriverState *bs,

Why does this need to be an external block layer interface? (block.h
instead of block_int.h)

Or in fact...

> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> index b1d5237..379ce8d 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -152,7 +152,7 @@ static int raw_get_info(BlockDriverState *bs, 
> BlockDriverInfo *bdi)
> 
>  static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>  {
> -    bs->bl = bs->file->bs->bl;
> +    bdrv_merge_limits(&bs->bl, &bs->file->bs->bl);
>  }

...isn't this completely redundant because bdrv_refresh_limits() already
called bdrv_merge_limits(&bs->bl, &bs->file->bs->bl)? We could simply
remove the .bdrv_refresh_limits implementation from raw_bsd.

And then bdrv_merge_limits() could even be static (I think factoring it
out is a good idea anyway because it removes some duplication).

Kevin