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 <mre...@redhat.com>
> ---
> 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
