On 09/25/2012 02:12 PM, Eric Blake wrote: > On 09/25/2012 10:29 AM, Jeff Cody wrote: >> This adds the live commit coroutine. This iteration focuses on the >> commit only below the active layer, and not the active layer itself. >> >> The behaviour is similar to block streaming; the sectors are walked >> through, and anything that exists above 'base' is committed back down >> into base. At the end, intermediate images are deleted, and the >> chain stitched together. Images are restored to their original open >> flags upon completion. >> >> Signed-off-by: Jeff Cody <jc...@redhat.com> > >> +void commit_start(BlockDriverState *bs, BlockDriverState *base, >> + BlockDriverState *top, int64_t speed, >> + BlockErrorAction on_error, BlockDriverCompletionFunc *cb, >> + void *opaque, Error **errp) >> +{ >> + CommitBlockJob *s; >> + BlockReopenQueue *reopen_queue = NULL; >> + int orig_overlay_flags; >> + int orig_base_flags; >> + BlockDriverState *overlay_bs; >> + Error *local_err = NULL; >> + >> + if ((on_error == BLOCK_ERR_STOP_ANY || >> + on_error == BLOCK_ERR_STOP_ENOSPC) && >> + !bdrv_iostatus_is_enabled(bs)) { >> + error_set(errp, QERR_INVALID_PARAMETER_COMBINATION); >> + return; >> + } >> + >> + overlay_bs = bdrv_find_overlay(bs, top); >> + >> + if (overlay_bs == NULL) { >> + error_setg(errp, "Could not find overlay image for %s:", >> top->filename); >> + return; >> + } >> + >> + orig_base_flags = bdrv_get_flags(base); >> + orig_overlay_flags = bdrv_get_flags(overlay_bs); > > I think you are missing a check here that base is on the backing chain > of top. See also my comments to 5/7. >
Did you mean your comments on 6/7 (or am I missing an email)? This does get partially validated in patch 5/7, in the qmp_block_commit() handler - both base and top are verified to be in the chain 'bs'. What is not validated, however, is that you did not swap your 'top' and 'base' arguments. I'll add a check here for that, to make sure that base is reachable from overlay_bs. >> + >> + /* convert base_bs & overlay_bs to r/w, if necessary */ >> + if (!(orig_base_flags & BDRV_O_RDWR)) { >> + reopen_queue = bdrv_reopen_queue(reopen_queue, base, >> + orig_base_flags | BDRV_O_RDWR); >> + } >> + if (!(orig_overlay_flags & BDRV_O_RDWR)) { >> + reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, >> + orig_overlay_flags | BDRV_O_RDWR); >> + } >> + if (reopen_queue) { >> + bdrv_reopen_multiple(reopen_queue, &local_err); > > Is it valid to make a no-op call, such as: > > { "execute":"block-commit", "arguments":{ > "device":"drive0", "top":"base", "base":"base" }} > > If so, should we do an early exit here, rather than temporarily changing > base to R/W just to change it back to R/O? > > If not, should we be rejecting it up front (again, back to the question > of failing if 'base' is not a backing file of 'top', even if both 'top' > and 'base' are backing files of 'device'). > I'll add a quick check for that as well. Patch 5 checks for it in one scenario (default 'top'), and returns a generic error of: "Invalid files for merge: top and base are the same" I'll make sure to check for that in all scenarios (or just make the test mentioned earlier incorporate top == base as well) Thanks, Jeff