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

[Kevin Wolf <kwolf@redhat.com>]

Am 08.10.2015 um 12:23 hat Fam Zheng geschrieben:
> On Thu, 10/01 15:13, Kevin Wolf wrote:
> > @@ -1935,6 +1932,11 @@ void bdrv_close(BlockDriverState *bs)
> >              bdrv_unref(backing_hd);
> >          }
> >  
> > +        if (bs->file != NULL) {
> > +            bdrv_unref_child(bs, bs->file);
> > +            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 */
> > @@ -1946,7 +1948,6 @@ void bdrv_close(BlockDriverState *bs)
> >  
> >          g_free(bs->opaque);
> >          bs->opaque = NULL;
> > -        bs->drv = NULL;
> >          bs->copy_on_read = 0;
> >          bs->backing_file[0] = '\0';
> >          bs->backing_format[0] = '\0';
> > @@ -1959,11 +1960,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;
> > -        }
> 
> Why do you need to move them up? Changing bdrv_unref to bdrv_unref_child is not
> enough?

I think it conflicted with the foreach loop above. Technically it might
have worked if I left it as bdrv_unref() in its original place because
the loop only calls bdrv_detach_child(), but with the intention of
replacing the detach with an unref I think it makes more sense to do a
proper bdrv_unref_child() before the loop.

> > 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 */
> 
> Isn't "backing file" a confusing term for bs->file given that we have
> bs->backing_hd? :)

I guess it is, even though blkdebug doesn't use bs->backing_hd. Feel
free to send a patch.

> > -    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) {
> 
> Should we keep the assertion?

The assertion was there to ensure that a new BDS is created instead of
reusing an existing one. bdrv_open_child() always creates a new one.

> > +    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;
> >      }

Kevin