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

[Kevin Wolf]

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?

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

> +    } 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)

Kevin