From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50639) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bAZK7-0007W8-Lr for qemu-devel@nongnu.org; Wed, 08 Jun 2016 04:58:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bAZK1-0004cR-MZ for qemu-devel@nongnu.org; Wed, 08 Jun 2016 04:58:50 -0400 Date: Wed, 8 Jun 2016 10:58:35 +0200 From: Kevin Wolf Message-ID: <20160608085835.GB5324@noname.str.redhat.com> References: <20160606144212.24074-1-mreitz@redhat.com> <20160606144212.24074-2-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160606144212.24074-2-mreitz@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 1/3] block: Allow replacement of a BDS by its overlay List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, Fam Zheng Am 06.06.2016 um 16:42 hat Max Reitz geschrieben: > change_parent_backing_link() asserts that the BDS to be replaced is not > used as a backing file. However, we may want to replace a BDS by its > overlay in which case that very link should not be redirected. > > Signed-off-by: Max Reitz So the scenario is like this? +---- to v base <- from <- top And we want to change it into this: base <- from <- to <- top Okay, makes sense. (Hm, put ASCII art in the commit message? I'd be all for it.) > block.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/block.c b/block.c > index f54bc25..16463aa 100644 > --- a/block.c > +++ b/block.c > @@ -2224,9 +2224,22 @@ void bdrv_close_all(void) > static void change_parent_backing_link(BlockDriverState *from, > BlockDriverState *to) > { > - BdrvChild *c, *next; > + BdrvChild *c, *next, *to_c; > > QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) { > + if (c->role == &child_backing) { > + /* Allow @from to be in a backing chain, but only if it is @to's > + * backing chain. Do not replace @from by @to there. */ The comment suggests bdrv_chain_contains(), but you only accept it as a direct child of top and you accept non-backing-file children. Intuitively I would say that anywhere in the backing chain would make sense, but we can always allow that later when we actually need it. Accepting all types of children sounds right when I think about inserting a quorum node as 'to'. So I guess the code is fine and the comment should be changed to correctly reflect what the code does. > + QLIST_FOREACH(to_c, &to->children, next) { > + if (to_c == c) { > + break; > + } > + } > + if (to_c) { > + continue; > + } > + } > + > assert(c->role != &child_backing); > bdrv_ref(to); > bdrv_replace_child(c, to); The other thing is that I'm unsure whether this function makes any sense at all. "Replace in all parents" is kind of arbitrary. In the long term, we may want to allow the user to specify the exact graph modifications on (block-)job-complete. Kevin