Re: [Qemu-devel] [PATCH v13 08/14] qemu-img: Implement commit like QMP

[Max Reitz]

On 2014-10-23 at 13:59, Kevin Wolf wrote:
> Am 22.10.2014 um 14:51 hat Max Reitz geschrieben:
> > 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.
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > ---
> >   block/Makefile.objs |  3 +-
> >   qemu-img.c          | 82 ++++++++++++++++++++++++++++++++++++++++-------------
> >   2 files changed, 64 insertions(+), 21 deletions(-)
> >
> > diff --git a/block/Makefile.objs b/block/Makefile.objs
> > index 27911b6..04b0e43 100644
> > --- a/block/Makefile.objs
> > +++ b/block/Makefile.objs
> > @@ -9,7 +9,7 @@ block-obj-y += block-backend.o snapshot.o qapi.o
> >   block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
> >   block-obj-$(CONFIG_POSIX) += raw-posix.o
> >   block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
> > -block-obj-y += null.o
> > +block-obj-y += null.o mirror.o
> >   
> >   block-obj-y += nbd.o nbd-client.o sheepdog.o
> >   block-obj-$(CONFIG_LIBISCSI) += iscsi.o
> > @@ -23,7 +23,6 @@ block-obj-y += accounting.o
> >   
> >   common-obj-y += stream.o
> >   common-obj-y += commit.o
> > -common-obj-y += mirror.o
> >   common-obj-y += backup.o
> >   
> >   iscsi.o-cflags     := $(LIBISCSI_CFLAGS)
> > diff --git a/qemu-img.c b/qemu-img.c
> > index 09e7e72..f1f2857 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -31,6 +31,7 @@
> >   #include "sysemu/sysemu.h"
> >   #include "sysemu/block-backend.h"
> >   #include "block/block_int.h"
> > +#include "block/blockjob.h"
> >   #include "block/qapi.h"
> >   #include <getopt.h>
> >   
> > @@ -715,13 +716,47 @@ fail:
> >       return ret;
> >   }
> >   
> > +typedef struct CommonBlockJobCBInfo {
> > +    BlockDriverState *bs;
> > +    Error **errp;
> > +} CommonBlockJobCBInfo;
> > +
> > +static void common_block_job_cb(void *opaque, int ret)
> > +{
> > +    CommonBlockJobCBInfo *cbi = opaque;
> > +
> > +    if (ret < 0) {
> > +        error_setg_errno(cbi->errp, -ret, "Block job failed");
> > +    }
> > +
> > +    /* Drop this block job's reference */
> > +    bdrv_unref(cbi->bs);
> > +}
> > +
> > +static void run_block_job(BlockJob *job, Error **errp)
> > +{
> > +    AioContext *aio_context = bdrv_get_aio_context(job->bs);
> > +
> > +    do {
> > +        aio_poll(aio_context, true);
> > +
> > +        if (!job->busy && !job->ready) {
> > +            block_job_resume(job);
> > +        }
> I wasn't quite sure what this is for. With BLOCKDEV_ON_ERROR_REPORT, why
> would the job ever be paused?

I remember you telling that I puzzled this code together until it 
worked. At one point in time, it didn't work without this. It works now. 
I'm going to trust you.

> While trying out what would happen with failing requests (both for
> checking this and what error message I would get), my image got
> corrupted (as I told you on IRC):
>
> $ qemu-img create -f qcow2 -b /tmp/backing.qcow2 /tmp/test.qcow2 64M
> $ qemu-img commit blkdebug::/tmp/test.qcow2
> qemu-img: Could not empty blkdebug::/tmp/test.qcow2: Operation not supported
> $ qemu-io -c 'write 0 1M' /tmp/test.qcow2
> qcow2: Marking image as corrupt: Preventing invalid write on metadata
> (overlaps with active L1 table); further corruption events will be
> suppressed
> write failed: Input/output error

As discussed on IRC, this is due to qcow2 marking every image clean when 
it's closed. Setting bs->drv to NULL solves this issue.

> > +    } while (!job->ready);
> > +
> > +    block_job_complete_sync(job, errp);
> > +}
> > +
> >   static int img_commit(int argc, char **argv)
> >   {
> >       int c, ret, flags;
> >       const char *filename, *fmt, *cache;
> >       BlockBackend *blk;
> > -    BlockDriverState *bs;
> > +    BlockDriverState *bs, *base_bs;
> >       bool quiet = false;
> > +    Error *local_err = NULL;
> > +    CommonBlockJobCBInfo cbi;
> >   
> >       fmt = NULL;
> >       cache = BDRV_DEFAULT_CACHE;
> > @@ -764,29 +799,38 @@ static int img_commit(int argc, char **argv)
> >       }
> >       bs = blk_bs(blk);
> >   
> > -    ret = bdrv_commit(bs);
> > -    switch(ret) {
> > -    case 0:
> > -        qprintf(quiet, "Image committed.\n");
> > -        break;
> > -    case -ENOENT:
> > -        error_report("No disk inserted");
> > -        break;
> > -    case -EACCES:
> > -        error_report("Image is read-only");
> > -        break;
> > -    case -ENOTSUP:
> > -        error_report("Image is already committed");
> > -        break;
> > -    default:
> > -        error_report("Error while committing image");
> > -        break;
> > +    /* This is different from QMP, which by default uses the deepest file in 
> > the
> > +     * backing chain (i.e., the very base); however, the traditional behavior 
> > of
> > +     * qemu-img commit is using the immediate backing file. */
> > +    base_bs = bs->backing_hd;
> > +    if (!base_bs) {
> > +        error_set(&local_err, QERR_BASE_NOT_FOUND, "NULL");
> $ qemu-img create -f qcow2 /tmp/test.qcow2 64M
> Formatting '/tmp/test.qcow2', fmt=qcow2 size=67108864 encryption=off 
> cluster_size=65536 lazy_refcounts=off
> $ qemu-img commit /tmp/test.qcow2
> qemu-img: Base 'NULL' not found
>
> Do we expect any user to understand what we want to tell them? This
> should clearly be something along the lines of error_setg(&local_err,
> "Image doesn't have a backing file"). (Before this patch, it said
> "Image is already committed", which isn't great either, but not as bad
> as the new message)

Yes, will fix.

Max