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
