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

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3523 bytes --]

Am 22.09.2015 um 20:36 hat Max Reitz geschrieben:
> 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 <kwolf@redhat.com>
> > ---
> >  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 <mreitz@redhat.com>
> 
> > 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.

Seems safe to do, and reads a bit nicer, so I'll change the patch
accordingly. Thanks.

Kevin

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



[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]