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

[Kevin Wolf]

Am 13.10.2015 um 03:33 hat Jeff Cody geschrieben:
> On Fri, Oct 09, 2015 at 02:15:30PM +0200, 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>
> > Reviewed-by: Max Reitz <address@hidden>
> > Reviewed-by: Alberto Garcia <address@hidden>
> > Reviewed-by: Fam Zheng <address@hidden>
> > ---
> >  block.c                   | 63 
> > ++++++++++++++++++++++-------------------------
> >  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, 378 insertions(+), 344 deletions(-)
> >
> 
> [snip lots of code]
> 
> > diff --git a/block/blkdebug.c b/block/blkdebug.c
> > index bc247f4..117fce6 100644
> > --- a/block/blkdebug.c
> > +++ b/block/blkdebug.c
> > @@ -427,10 +427,10 @@ static int blkdebug_open(BlockDriverState *bs, QDict 
> > *options, int flags,
> >      s->state = 1;
> >  
> >      /* Open the backing file */
> > -    assert(bs->file == NULL);
> > -    ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-image"), 
> > options, "image",
> > -                          bs, &child_file, false, &local_err);
> > -    if (ret < 0) {
> > +    bs->file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options, 
> > "image",
> > +                               bs, &child_file, false, &local_err);
> > +    if (local_err) {
> > +        ret = -EINVAL;
> >          error_propagate(errp, local_err);
> >          goto out;
> >      }
> > @@ -449,7 +449,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict 
> > *options, int flags,
> >      goto out;
> >  
> >  fail_unref:
> > -    bdrv_unref(bs->file);
> > +    bdrv_unref(bs->file->bs);
> 
> 
> Would it be better to use bdrv_unref_child() here?

Good catch, without it the BdrvChild object is leaked here. (And
blkverify already leaks the whole BDS on .bdrv_open failure. I'll send a
separate patch on top of this series for that.)

I'll just fix this (and your coding style points) while merging the
series; unless something major comes up, I'm not going to send another
version.

Kevin