From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46730) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZDavt-0003Lc-IS for qemu-devel@nongnu.org; Fri, 10 Jul 2015 12:13:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZDavs-0002YJ-In for qemu-devel@nongnu.org; Fri, 10 Jul 2015 12:13:49 -0400 References: <1436384203-10576-1-git-send-email-kwolf@redhat.com> <1436384203-10576-6-git-send-email-kwolf@redhat.com> From: Max Reitz Message-ID: <559FEF31.6070708@redhat.com> Date: Fri, 10 Jul 2015 18:13:37 +0200 MIME-Version: 1.0 In-Reply-To: <1436384203-10576-6-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH for-2.4 5/5] block: Fix backing file child when modifying graph List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-block@nongnu.org Cc: berto@igalia.com, qemu-devel@nongnu.org, stefanha@redhat.com On 08.07.2015 21:36, Kevin Wolf wrote: > This patch moves bdrv_attach_child() from the individual places that add > a backing file to a BDS to bdrv_set_backing_hd(), which is called by all > of them. It also adds bdrv_detach_child() there. > > For normal operation (starting with one backing file chain and not > changing it until the topmost image is closed) and live snapshots, this > constitutes no change in behaviour. > > For all other cases, this is a fix for the bug that the old backing file > was still referenced as a child, and the new one wasn't referenced. > > Signed-off-by: Kevin Wolf > --- > block.c | 5 +++-- > include/block/block_int.h | 1 + > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/block.c b/block.c > index d5c9f03..d088ee0 100644 > --- a/block.c > +++ b/block.c > @@ -1141,6 +1141,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) > if (bs->backing_hd) { > assert(bs->backing_blocker); > bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker); > + bdrv_detach_child(bs->backing_child); > } else if (backing_hd) { > error_setg(&bs->backing_blocker, > "node is used as backing hd of '%s'", > @@ -1151,8 +1152,10 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) > if (!backing_hd) { > error_free(bs->backing_blocker); > bs->backing_blocker = NULL; > + bs->backing_child = NULL; I'd prefer this to be directly below the bdrv_detach_child() call, but that's just a question of personal preference. Reviewed-by: Max Reitz > goto out; > } > + bs->backing_child = bdrv_attach_child(bs, backing_hd, &child_backing); > bs->open_flags &= ~BDRV_O_NO_BACKING; > pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename); > pstrcpy(bs->backing_format, sizeof(bs->backing_format), > @@ -1236,7 +1239,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) > goto free_exit; > } > > - bdrv_attach_child(bs, backing_hd, &child_backing); > bdrv_set_backing_hd(bs, backing_hd); > > free_exit: > @@ -2171,7 +2173,6 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top) > /* The contents of 'tmp' will become bs_top, as we are > * swapping bs_new and bs_top contents. */ > bdrv_set_backing_hd(bs_top, bs_new); > - bdrv_attach_child(bs_top, bs_new, &child_backing); > } > > static void bdrv_delete(BlockDriverState *bs) > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 8996baf..2cc973c 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -379,6 +379,7 @@ struct BlockDriverState { > char exact_filename[PATH_MAX]; > > BlockDriverState *backing_hd; > + BdrvChild *backing_child; > BlockDriverState *file; > > NotifierList close_notifiers; >