From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56560) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V28Hz-0008Dk-4x for qemu-devel@nongnu.org; Wed, 24 Jul 2013 19:16:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V28Hv-0001ct-7U for qemu-devel@nongnu.org; Wed, 24 Jul 2013 19:16:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55737) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V28Hu-0001cg-Vb for qemu-devel@nongnu.org; Wed, 24 Jul 2013 19:16:07 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r6ONG5P7017895 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 24 Jul 2013 19:16:05 -0400 Date: Wed, 24 Jul 2013 19:16:02 -0400 From: Jeff Cody Message-ID: <20130724231602.GA19811@localhost.localdomain> References: <1374054136-28741-1-git-send-email-famz@redhat.com> <1374054136-28741-9-git-send-email-famz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1374054136-28741-9-git-send-email-famz@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 08/11] block: simplify bdrv_drop_intermediate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: kwolf@redhat.com, hbrock@redhat.com, qemu-devel@nongnu.org, rjones@redhat.com, imain@redhat.com, stefanha@redhat.com, pbonzini@redhat.com On Wed, Jul 17, 2013 at 05:42:13PM +0800, Fam Zheng wrote: > bdrv_drop_intermediate used a local list to iterate through backing > chain and delete each BDS. It is simplified while adopting to refcount > mechanism. > Hi Fam, The reason for the local list is to keep the BDS deletion transactional, so it can be rolled back in case of error (see below) > Signed-off-by: Fam Zheng > --- > block.c | 71 ++++++++++------------------------------------------------------- > 1 file changed, 11 insertions(+), 60 deletions(-) > > diff --git a/block.c b/block.c > index 57a3876..499de22 100644 > --- a/block.c > +++ b/block.c > @@ -2027,12 +2027,6 @@ BlockDriverState *bdrv_find_overlay(BlockDriverState *active, > return overlay; > } > > -typedef struct BlkIntermediateStates { > - BlockDriverState *bs; > - QSIMPLEQ_ENTRY(BlkIntermediateStates) entry; > -} BlkIntermediateStates; > - > - > /* > * Drops images above 'base' up to and including 'top', and sets the image > * above 'top' to have base as its backing file. > @@ -2062,15 +2056,9 @@ typedef struct BlkIntermediateStates { > int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top, > BlockDriverState *base) > { > - BlockDriverState *intermediate; > - BlockDriverState *base_bs = NULL; > BlockDriverState *new_top_bs = NULL; > - BlkIntermediateStates *intermediate_state, *next; > int ret = -EIO; > > - QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete; > - QSIMPLEQ_INIT(&states_to_delete); > - > if (!top->drv || !base->drv) { > goto exit; > } > @@ -2082,58 +2070,21 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top, > goto exit; > } > > - /* 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; > - } > - > - intermediate = top; > - > - /* 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; > + while (new_top_bs->backing_hd && new_top_bs->backing_hd != base) { > + BlockDriverState *backing = new_top_bs->backing_hd; > + if (backing == NULL) { > + goto exit; If you simplify it until just a while loop that unrefs/deletes the BDS inside the loop as you navigate the chain, then any error exit leaves you in a bad state, with a potentially invalid chain. This is one such error potential. > } > - 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; > + new_top_bs->backing_hd = backing->backing_hd; > + /* break backing_hd chain before releasing bs, so we don't free all the > + * way up the backing chain */ > + backing->backing_hd = NULL; > + bdrv_unref(backing, false); These two statements, which unlink this BDS from the chain, can't be undone now, in case of error. > } > > - /* 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 : ""); > - if (ret) { > - goto exit; > - } > - if (new_top_bs->backing_hd) { > - bdrv_unref(new_top_bs->backing_hd, false); > - } > - new_top_bs->backing_hd = base_bs; > - bdrv_ref(base_bs, false); > - > - QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) { > - /* so that bdrv_close() does not recursively close the chain */ > - intermediate_state->bs->backing_hd = NULL; > - bdrv_delete(intermediate_state->bs); > - } The foreach loop over the list was placed such that there were no more error paths; we were guaranteed at this point to have been able delete and unchain each intermediate BDS. > - ret = 0; > - > + ret = bdrv_change_backing_file(new_top_bs, base->filename, > + base->drv ? base->drv->format_name : ""); This is effectively another error path that would cause problems, if ret < 0. > exit: > - QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) { > - g_free(intermediate_state); > - } > return ret; > } > > -- > 1.8.3.2 > >