| Message ID | e9ac3c39bf16a9f6e287a75f8332598acef51e9f.1343710713.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/30/2012 11:16 PM, Jeff Cody wrote: > Add bdrv_find_image(), bdrv_find_base(), and bdrv_delete_intermediate(). > > bdrv_find_image(): given a filename and a BDS, find the image in the chain > that matches the passed filename. > > bdrv_find_base(): given a BDS, find the base image (parent-most image) > > 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. A diagram, as was in your cover letter, would help (either here, or better yet in the comments describing this function in place)... > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > block.h | 4 ++ > 2 files changed, 139 insertions(+), 1 deletion(-) > > diff --git a/block.c b/block.c > index 522acd1..a3c8d33 100644 > --- a/block.c > +++ b/block.c > @@ -1408,7 +1408,7 @@ int bdrv_commit(BlockDriverState *bs) > > if (!drv) > return -ENOMEDIUM; > - > + > if (!bs->backing_hd) { > return -ENOTSUP; > } Spurious whitespace cleanup, since nothing else in this hunk belongs to you. Is that trailing whitespace present upstream, or was it introduced in one of the patches you based off of? > @@ -1661,6 +1661,110 @@ int bdrv_change_backing_file(BlockDriverState *bs, > return ret; > } > > +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. > + */ > +int bdrv_delete_intermediate(BlockDriverState *active, BlockDriverState *top, > + BlockDriverState *base) ...that is, I think this would aid the reader: Converts: bottom <- base <- intermediate <- top <- active to bottom <- base <- active where top==active is permitted > @@ -3128,6 +3232,36 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, > return NULL; > } > > +BlockDriverState *bdrv_find_image(BlockDriverState *bs, > + const char *filename) > +{ > + if (!bs || !bs->drv) { > + return NULL; > + } > + > + if (strcmp(bs->filename, filename) == 0) { > + return bs; > + } else { > + return bdrv_find_image(bs->backing_hd, filename); Tail-recursive; are we worried enough about ever hitting stack overflow due to a really deep change to convert this into a while loop recursion instead? [Probably not] > + } > +} > + > +BlockDriverState *bdrv_find_base(BlockDriverState *bs) > +{ > + BlockDriverState *curr_bs = NULL; > + > + if (!bs) { > + return NULL; > + } > + > + curr_bs = bs; > + > + while (curr_bs->backing_hd) { > + curr_bs = curr_bs->backing_hd; > + } Then again, here you did while-loop recursion, so using the same style in both functions might be worthwhile.
On 07/31/2012 01:34 PM, Eric Blake wrote: > On 07/30/2012 11:16 PM, Jeff Cody wrote: >> Add bdrv_find_image(), bdrv_find_base(), and bdrv_delete_intermediate(). >> >> bdrv_find_image(): given a filename and a BDS, find the image in the chain >> that matches the passed filename. >> >> bdrv_find_base(): given a BDS, find the base image (parent-most image) >> >> 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. > > A diagram, as was in your cover letter, would help (either here, or > better yet in the comments describing this function in place)... > Or even better, both :) I'll add that in for v1. >> >> Signed-off-by: Jeff Cody <jcody@redhat.com> >> --- >> block.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >> block.h | 4 ++ >> 2 files changed, 139 insertions(+), 1 deletion(-) >> >> diff --git a/block.c b/block.c >> index 522acd1..a3c8d33 100644 >> --- a/block.c >> +++ b/block.c >> @@ -1408,7 +1408,7 @@ int bdrv_commit(BlockDriverState *bs) >> >> if (!drv) >> return -ENOMEDIUM; >> - >> + >> if (!bs->backing_hd) { >> return -ENOTSUP; >> } > > Spurious whitespace cleanup, since nothing else in this hunk belongs to > you. Is that trailing whitespace present upstream, or was it introduced > in one of the patches you based off of? Likely a spurious cleanup - I had several trailing whitespaces in my block.c changes, and scripts/checkpatch.pl warned me of those. I cleaned them up, and I must have cleaned up an extra one with my regex. > >> @@ -1661,6 +1661,110 @@ int bdrv_change_backing_file(BlockDriverState *bs, >> return ret; >> } >> >> +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. >> + */ >> +int bdrv_delete_intermediate(BlockDriverState *active, BlockDriverState *top, >> + BlockDriverState *base) > > ...that is, I think this would aid the reader: > > Converts: > > bottom <- base <- intermediate <- top <- active > > to > > bottom <- base <- active > > where top==active is permitted I agree that is better. And, for clarity, bottom==base is permitted as well. > >> @@ -3128,6 +3232,36 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, >> return NULL; >> } >> >> +BlockDriverState *bdrv_find_image(BlockDriverState *bs, >> + const char *filename) >> +{ >> + if (!bs || !bs->drv) { >> + return NULL; >> + } >> + >> + if (strcmp(bs->filename, filename) == 0) { >> + return bs; >> + } else { >> + return bdrv_find_image(bs->backing_hd, filename); > > Tail-recursive; are we worried enough about ever hitting stack overflow > due to a really deep change to convert this into a while loop recursion > instead? [Probably not] Not too worried about it, because the chain should not be *that* long, and also the block layer handles the chain in a similar fashion other places, so we'll likely blow up in those places first :) That said, when doing some automated live snapshot testing with an obscene number of snapshots, I did manage to blow the stack (IIRC) in the recursive open. That was with something like a chain of 1500 snapshots, which seems a bit excessive. But, I agree with your point below: > >> + } >> +} >> + >> +BlockDriverState *bdrv_find_base(BlockDriverState *bs) >> +{ >> + BlockDriverState *curr_bs = NULL; >> + >> + if (!bs) { >> + return NULL; >> + } >> + >> + curr_bs = bs; >> + >> + while (curr_bs->backing_hd) { >> + curr_bs = curr_bs->backing_hd; >> + } > > Then again, here you did while-loop recursion, so using the same style > in both functions might be worthwhile. > Yes - maybe that is a good reason to have bdrv_find_image() be a while-loop (I based it off of bdrv_find_backing_image(), which is why it was recursive). In general, I find recursive functions make my brain hurt, so I tend to like while-loops better :)
diff --git a/block.c b/block.c index 522acd1..a3c8d33 100644 --- a/block.c +++ b/block.c @@ -1408,7 +1408,7 @@ int bdrv_commit(BlockDriverState *bs) if (!drv) return -ENOMEDIUM; - + if (!bs->backing_hd) { return -ENOTSUP; } @@ -1661,6 +1661,110 @@ int bdrv_change_backing_file(BlockDriverState *bs, return ret; } +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. + */ +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; + } + + /* special case of top->backing_hd already pointing to base - nothing + * to do, no intermediate images + */ + if (top->backing_hd == base) { + ret = 0; + goto exit; + } + + /* if the active bs layer is the same as the new top, then there + * is no image above the top, so it is also the new_top (and we must + * not delete it below, either) + */ + if (active == top) { + new_top_bs = active; + } else { + intermediate = active; + while (intermediate->backing_hd) { + if (intermediate->backing_hd == top) { + new_top_bs = intermediate; + break; + } + intermediate = intermediate->backing_hd; + } + } + + if (new_top_bs == NULL) { + /* 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; + + /* 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 : ""); + if (ret) { + goto exit; + } + new_top_bs->backing_hd = base_bs; + + + 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; + +exit: + QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) { + g_free(intermediate_state); + } + return ret; +} + + static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset, size_t size) { @@ -3128,6 +3232,36 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, return NULL; } +BlockDriverState *bdrv_find_image(BlockDriverState *bs, + const char *filename) +{ + if (!bs || !bs->drv) { + return NULL; + } + + if (strcmp(bs->filename, filename) == 0) { + return bs; + } else { + return bdrv_find_image(bs->backing_hd, filename); + } +} + +BlockDriverState *bdrv_find_base(BlockDriverState *bs) +{ + BlockDriverState *curr_bs = NULL; + + if (!bs) { + return NULL; + } + + curr_bs = bs; + + while (curr_bs->backing_hd) { + curr_bs = curr_bs->backing_hd; + } + return curr_bs; +} + #define NB_SUFFIXES 4 char *get_human_readable_size(char *buf, int buf_size, int64_t size) diff --git a/block.h b/block.h index 25e0230..70fa53c 100644 --- a/block.h +++ b/block.h @@ -183,6 +183,10 @@ int bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file, const char *backing_fmt); void bdrv_register(BlockDriver *bdrv); int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache); +BlockDriverState *bdrv_find_image(BlockDriverState *bs, const char *filename); +BlockDriverState *bdrv_find_base(BlockDriverState *bs); +int bdrv_delete_intermediate(BlockDriverState *active, BlockDriverState *top, + BlockDriverState *base); typedef struct BdrvCheckResult {
Add bdrv_find_image(), bdrv_find_base(), and bdrv_delete_intermediate(). bdrv_find_image(): given a filename and a BDS, find the image in the chain that matches the passed filename. bdrv_find_base(): given a BDS, find the base image (parent-most image) 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. Signed-off-by: Jeff Cody <jcody@redhat.com> --- block.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- block.h | 4 ++ 2 files changed, 139 insertions(+), 1 deletion(-)