Message ID | 1374054136-28741-9-git-send-email-famz@redhat.com |
---|---|
State | New |
Headers | show |
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 <famz@redhat.com> > --- > 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 > >
On Wed, 07/24 19:16, Jeff Cody wrote: > 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 <famz@redhat.com> > > --- > > 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. > Yes, I'll fix this.
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; } - 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); } - /* 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); - } - ret = 0; - + ret = bdrv_change_backing_file(new_top_bs, base->filename, + base->drv ? base->drv->format_name : ""); exit: - QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) { - g_free(intermediate_state); - } return ret; }
bdrv_drop_intermediate used a local list to iterate through backing chain and delete each BDS. It is simplified while adopting to refcount mechanism. Signed-off-by: Fam Zheng <famz@redhat.com> --- block.c | 71 ++++++++++------------------------------------------------------- 1 file changed, 11 insertions(+), 60 deletions(-)