On 04/07/2014 11:29 AM, Max Reitz wrote: > qemu-img should use QMP commands whenever possible in order to ensure > feature completeness of both online and offline image operations. As > qemu-img itself has no access to QMP (since this would basically require > just everything being linked into qemu-img), imitate QMP's > implementation of block-commit by using commit_active_start() and then > waiting for the block job to finish. > > This new implementation does not empty the snapshot image, as opposed to > the old implementation using bdrv_commit(). However, as QMP's > block-commit apparently never did this and as qcow2 (which is probably > qemu's standard image format) does not even implement the required > function (bdrv_make_empty()), it does not seem necessary. > > Signed-off-by: Max Reitz > --- > block/Makefile.objs | 2 +- > qemu-img.c | 68 ++++++++++++++++++++++++++++++++++++++--------------- > 2 files changed, 50 insertions(+), 20 deletions(-) > > @@ -728,29 +755,32 @@ static int img_commit(int argc, char **argv) > if (!bs) { > return 1; > } > + > + base_bs = bdrv_find_base(bs); > + if (!base_bs) { > + error_set(&local_err, QERR_BASE_NOT_FOUND, "NULL"); > + goto done; > + } Is it worth adding an optional '-b base' image to allow qemu-img to commit across multiple images? That is, QMP can shorten from 'a <- b <- c' all the way to 'a'; but qemu-img has to be called twice (once to 'a <- b' and second to 'a'). Separate commit, of course. > + > + commit_active_start(bs, base_bs, 0, COMMIT_BUF_SECTORS << BDRV_SECTOR_BITS, > + BLOCKDEV_ON_ERROR_REPORT, dummy_block_job_cb, bs, > + &local_err); > + if (error_is_set(&local_err)) { No new uses of error_is_set if we can help it. This can be 'if (local_err)'. > + goto done; > } > > + run_block_job(bs->job, &local_err); > + > +done: > bdrv_unref(bs); > - if (ret) { > + > + if (error_is_set(&local_err)) { and again. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org