Re: [Qemu-devel] [PATCH 05/16] block: Convert bs->file to BdrvChild

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> This patch removes the temporary duplication between bs->file and
> bs->file_child by converting everything to BdrvChild.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block.c                   | 61 ++++++++++++++++++++++------------------------
>  block/blkdebug.c          | 32 ++++++++++++------------
>  block/blkverify.c         | 33 ++++++++++++++-----------
>  block/bochs.c             |  8 +++---
>  block/cloop.c             | 10 ++++----
>  block/dmg.c               | 20 +++++++--------
>  block/io.c                | 50 +++++++++++++++++++-------------------
>  block/parallels.c         | 38 +++++++++++++++--------------
>  block/qapi.c              |  2 +-
>  block/qcow.c              | 42 +++++++++++++++++---------------
>  block/qcow2-cache.c       | 11 +++++----
>  block/qcow2-cluster.c     | 38 +++++++++++++++++------------
>  block/qcow2-refcount.c    | 45 ++++++++++++++++++----------------
>  block/qcow2-snapshot.c    | 30 ++++++++++++-----------
>  block/qcow2.c             | 62 
> ++++++++++++++++++++++++-----------------------
>  block/qed-table.c         |  4 +--
>  block/qed.c               | 32 ++++++++++++------------
>  block/raw_bsd.c           | 40 +++++++++++++++---------------
>  block/snapshot.c          | 12 ++++-----
>  block/vdi.c               | 17 +++++++------
>  block/vhdx-log.c          | 25 ++++++++++---------
>  block/vhdx.c              | 36 ++++++++++++++-------------
>  block/vmdk.c              | 27 +++++++++++----------
>  block/vpc.c               | 34 ++++++++++++++------------
>  include/block/block.h     |  8 +++++-
>  include/block/block_int.h |  3 +--
>  26 files changed, 377 insertions(+), 343 deletions(-)

Reviewed-by: Max Reitz <address@hidden>

> diff --git a/block.c b/block.c
> index 2e9e5e2..93d713b 100644
> --- a/block.c
> +++ b/block.c

[...]

> @@ -1929,6 +1925,11 @@ void bdrv_close(BlockDriverState *bs)
>              bdrv_unref(backing_hd);
>          }
>  
> +        if (bs->file != NULL) {
> +            bdrv_unref(bs->file->bs);

I think we can use bdrv_unref_child(bs->file) here. I'd personally like
it more, at least (because using bdrv_unref() and relying on the loop
below is basically what the comment inside of the loop advises against).

Yes, I know, eventually, we want to drop this block anyway and let the
loop below handle everything using bdrv_unref_child(). But when we do
that conversion, it'll be obvious to drop a bdrv_unref_child() call,
whereas dropping bdrv_unref() alone isn't so obvious.

Max

> +            bs->file = NULL;
> +        }
> +
>          QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
>              /* TODO Remove bdrv_unref() from drivers' close function and use
>               * bdrv_unref_child() here */
> @@ -1953,11 +1954,6 @@ void bdrv_close(BlockDriverState *bs)
>          bs->options = NULL;
>          QDECREF(bs->full_open_options);
>          bs->full_open_options = NULL;
> -
> -        if (bs->file != NULL) {
> -            bdrv_unref(bs->file);
> -            bs->file = NULL;
> -        }
>      }
>  
>      if (bs->blk) {

signature.asc
Description: OpenPGP digital signature