From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55474) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZBQ4o-0004NE-G0 for qemu-devel@nongnu.org; Sat, 04 Jul 2015 12:14:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZBQ4n-00026E-Dk for qemu-devel@nongnu.org; Sat, 04 Jul 2015 12:14:02 -0400 References: <0d86a074935ca6c1a0645e5d4af492c7cac12821.1435758248.git.berto@igalia.com> <55940FCC.1010206@redhat.com> From: Max Reitz Message-ID: <5598063F.5090006@redhat.com> Date: Sat, 4 Jul 2015 18:13:51 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/1] block: update BlockDriverState's children in bdrv_set_backing_hd() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia , qemu-devel@nongnu.org Cc: kwolf@redhat.com, qemu-block@nongnu.org, stefanha@redhat.com On 01.07.2015 21:41, Alberto Garcia wrote: > On Wed 01 Jul 2015 06:05:32 PM CEST, Max Reitz wrote: > >>> @@ -1120,6 +1128,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) >>> bs->backing_blocker = NULL; >>> goto out; >>> } >>> + >>> + bdrv_attach_child(bs, backing_hd, &child_backing); >>> + backing_hd->inherits_from = bs; >>> + backing_hd->open_flags = child_backing.inherit_flags(bs->open_flags); >> >> Do we really want this, unconditionally? ... After looking through the >> code, I can't find a place where we wouldn't. It just seems strange to >> have it here. > > Yeah, I understand. In general I think that the API for handling > bs->children is rather unclear and I wanted to avoid that callers need > to call bdrv_set_backing_hd() and bdrv_attach/detach_child() separately. Oh, sorry, I was unclear. The bdrv_attach_child() is fine, but I find unconditionally inheriting the flags from the backed BDS strange. >>> @@ -1332,7 +1345,16 @@ static void bdrv_attach_child(BlockDriverState *parent_bs, >>> BlockDriverState *child_bs, >>> const BdrvChildRole *child_role) >>> { >>> - BdrvChild *child = g_new(BdrvChild, 1); >>> + BdrvChild *child; >>> + >>> + /* Don't attach the child if it's already attached */ >>> + QLIST_FOREACH(child, &parent_bs->children, next) { >>> + if (child->bs == child_bs) { >>> + return; >>> + } >>> + } >> >> Hm, it may have been attached with a different role, though... I guess >> that would be a bug, however. But if it's the same role, trying to >> re-attach it seems wrong, too. So where could this happen? > > The reason I'm doing this is because of bdrv_open_backing_file(). That > function attaches the backing file to the parent file twice: once in > bdrv_open_inherit() and the second time in bdrv_set_backing_hd(). Okay, that's fine then. > One alternative would be not to attach it in bdrv_set_backing_hd(), but > since that function is used in many other places we would have to add > new calls to bdrv_attach_child() everywhere. > > That's one example of the situation I mentioned earlier: it seems > logical that bdrv_set_backing_hd() and bdrv_attach_child() go together, > but none of the solutions that came to my mind feels 100% right. I think putting it into bdrv_set_backing_hd() is fine. Still feeling a bit bad about overwriting the backing BDS's flags and making it inherit its flags from the backed BDS in bdrv_set_backing_hd(), but anyway: Reviewed-by: Max Reitz (I do think it is fine and can't think of any better solution)