Re: [Qemu-devel] [PATCH 3/4] qemu-img: Implement commit like QMP

[Max Reitz]

On 07.04.2014 21:10, Eric Blake wrote:
> 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 <address@hidden>
> > ---
> >   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.

Sounds interesting, I'll have a look.

> > +
> > +    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)'.

Okay, seems like I missed something.

Max

> > +        goto done;
> >       }
> >   
> > +    run_block_job(bs->job, &local_err);
> > +
> > +done:
> >       bdrv_unref(bs);
> > -    if (ret) {
> > +
> > +    if (error_is_set(&local_err)) {
> and again.