From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:32858) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T9c3n-00086f-A2 for qemu-devel@nongnu.org; Thu, 06 Sep 2012 09:24:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T9c3g-0007Bh-O2 for qemu-devel@nongnu.org; Thu, 06 Sep 2012 09:23:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46259) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T9c3g-0007BO-FY for qemu-devel@nongnu.org; Thu, 06 Sep 2012 09:23:48 -0400 Message-ID: <5048A3C5.7000000@redhat.com> Date: Thu, 06 Sep 2012 15:23:17 +0200 From: Kevin Wolf MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC v2 PATCH 1/6] block: add support functions for live commit, to find and delete images. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jeff Cody Cc: supriyak@linux.vnet.ibm.com, pbonzini@redhat.com, eblake@redhat.com, qemu-devel@nongnu.org, stefanha@gmail.com Am 30.08.2012 20:47, schrieb Jeff Cody: > Add bdrv_find_child(), and bdrv_delete_intermediate(). > > bdrv_find_child(): given 'bs' and the active (topmost) BDS of an image chain, > find the image that is the immediate top of 'bs' > > bdrv_delete_intermediate(): > Given 3 BDS (active, top, base), delete images above > base up to and including top, and set base to be the > parent of top's child node. > > E.g., this converts: > > bottom <- base <- intermediate <- top <- active > > to > > bottom <- base <- active > > where top == active is permitted, although active > will not be deleted. > > Signed-off-by: Jeff Cody At first, when just reading the function name, I thought this would actually delete the image file. Of course, it only removes it from the backing file chain, but leaves the image file around. I don't have a good suggestion, but if someone has a better name, I think we should change it. > --- > block.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > block.h | 5 ++- > 2 files changed, 146 insertions(+), 1 deletion(-) > > diff --git a/block.c b/block.c > index 9470319..11e275c 100644 > --- a/block.c > +++ b/block.c > @@ -1752,6 +1752,148 @@ int bdrv_change_backing_file(BlockDriverState *bs, > return ret; > } > > +/* Finds the image layer immediately to the 'top' of bs. > + * > + * active is the current topmost image. > + */ Most other function header comments in block.c have the /* on its own line, so it would be nice to stay consistent. Without looking at the code yet, I'm not quite sure if I understand this correctly. Could use an example: base <- a <- b <- c <- d bdrv_find_child(d, a) would return b? Why is it called bdrv_find_child then, b is neither a (direct) child of d nor a. > +BlockDriverState *bdrv_find_child(BlockDriverState *active, > + BlockDriverState *bs) > +{ > + BlockDriverState *child = NULL; > + BlockDriverState *intermediate; > + > + /* if the active bs layer is the same as the new top, then there > + * is no image above the top, so it will be returned as the child > + */ "new top"? And should this be mentioned in the header comment? Not sure how generally useful this behaviour is. If it's only the right thing for the only caller, maybe returning NULL and handling the case in the caller would be better. > + if (active == bs) { > + child = active; > + } else { > + intermediate = active; > + while (intermediate->backing_hd) { > + if (intermediate->backing_hd == bs) { > + child = intermediate; > + break; > + } > + intermediate = intermediate->backing_hd; > + } > + } > + > + return child; > +} So it returns NULL when bs isn't in the backing file chain of active. Probably worth mentioning in the comment as well. > + > +typedef struct BlkIntermediateStates { > + BlockDriverState *bs; > + QSIMPLEQ_ENTRY(BlkIntermediateStates) entry; > +} BlkIntermediateStates; > + > + > +/* deletes images above 'base' up to and including 'top', and sets the image > + * above 'top' to have base as its backing file. > + * > + * E.g., this will convert the following chain: > + * bottom <- base <- intermediate <- top <- active > + * > + * to > + * > + * bottom <- base <- active > + * > + * It is allowed for bottom==base, in which case it converts: > + * > + * base <- intermediate <- top <- active > + * > + * to > + * > + * base <- active > + * > + * It is also allowed for top==active, except in that case active is not > + * deleted: Hm, makes the interface inconsistent. Shouldn't you be using top == intermediate and it would work without any special casing? > + * > + * base <- intermediate <- top > + * > + * becomes > + * > + * base <- top > + */ > +int bdrv_delete_intermediate(BlockDriverState *active, BlockDriverState *top, > + BlockDriverState *base) > +{ > + BlockDriverState *intermediate; > + BlockDriverState *base_bs = NULL; > + BlockDriverState *new_top_bs = NULL; > + BlkIntermediateStates *intermediate_state, *next; > + int ret = -1; > + > + QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete; > + QSIMPLEQ_INIT(&states_to_delete); > + > + if (!top->drv || !base->drv) { > + goto exit; > + } > + > + new_top_bs = bdrv_find_child(active, top); new_top_bs is the first BDS that should not be removed from the chain. Why do we pass top and then search for new_top instead of passing new_top directly? > + > + /* special case of new_top_bs->backing_hd already pointing to base - nothing > + * to do, no intermediate images > + */ > + if (new_top_bs->backing_hd == base) { > + ret = 0; > + goto exit; > + } > + > + if (new_top_bs == NULL) { Never reached, we segfault before. (How about unit tests for this function, in the style of tests/check-q*.c?) > + /* we could not find the image above 'top', this is an error */ > + goto exit; > + } > + > + /* if the active and top image passed in are the same, then we > + * can't delete the active, so we start one below > + */ > + intermediate = (active == top) ? active->backing_hd : top; Aha. So intermediate is used to undo the special case. Now we're always on the last image to be deleted. This is equivalent to an unconditional new_top_bs->backing_hd. > + > + /* now we will go down through the list, and add each BDS we find > + * into our deletion queue, until we hit the 'base' > + */ > + while (intermediate) { > + intermediate_state = g_malloc0(sizeof(BlkIntermediateStates)); > + intermediate_state->bs = intermediate; > + QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, entry); > + > + if (intermediate->backing_hd == base) { > + base_bs = intermediate->backing_hd; > + break; > + } > + intermediate = intermediate->backing_hd; > + } > + if (base_bs == NULL) { > + /* something went wrong, we did not end at the base. safely > + * unravel everything, and exit with error */ > + goto exit; > + } > + > + /* success - we can delete the intermediate states, and link top->base */ > + ret = bdrv_change_backing_file(new_top_bs, base_bs->filename, > + base_bs->drv ? base_bs->drv->format_name : ""); Requires that new_top_bs is opened r/w. Definitely worth documenting. Kevin