From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34960) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZkVwm-0005k0-MO for qemu-devel@nongnu.org; Fri, 09 Oct 2015 07:34:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZkVwl-0000aG-PE for qemu-devel@nongnu.org; Fri, 09 Oct 2015 07:34:48 -0400 Date: Fri, 9 Oct 2015 13:34:36 +0200 From: Kevin Wolf Message-ID: <20151009113436.GC3956@noname.redhat.com> References: <1443705214-9304-1-git-send-email-kwolf@redhat.com> <1443705214-9304-6-git-send-email-kwolf@redhat.com> <20151008102301.GA10388@ad.nay.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151008102301.GA10388@ad.nay.redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 05/16] block: Convert bs->file to BdrvChild List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: berto@igalia.com, qemu-block@nongnu.org, armbru@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org, mreitz@redhat.com, stefanha@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