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

[Kevin Wolf]

Am 10.04.2014 um 16:32 hat Max Reitz geschrieben:
> On 08.04.2014 17:14, Kevin Wolf wrote:
> >Am 08.04.2014 um 14:50 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.
> >Leaves us with the HMP commit command that uses the old bdrv_commit()
> >function. I wonder if we can get rid of it by letting the HMP command
> >stop the VM, do a live commit, and then restart the VM.
> >
> >>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.
> >In fact, I think since qcow2 has discard support it would actually be
> >possible to write a sensible implementation of bdrv_make_empty(). That's
> >a separate feature, though, and can go in a different patch series.
> >
> >>Signed-off-by: Max Reitz <address@hidden>
> >>---
> >>  block/Makefile.objs |  2 +-
> >>  qemu-img.c          | 70 
> >> ++++++++++++++++++++++++++++++++++++++---------------
> >>  2 files changed, 52 insertions(+), 20 deletions(-)
> >>
> >>diff --git a/block/Makefile.objs b/block/Makefile.objs
> >>index fd88c03..2c37e80 100644
> >>--- a/block/Makefile.objs
> >>+++ b/block/Makefile.objs
> >>@@ -9,6 +9,7 @@ block-obj-y += 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 += mirror.o
> >>  ifeq ($(CONFIG_POSIX),y)
> >>  block-obj-y += nbd.o nbd-client.o sheepdog.o
> >>@@ -22,7 +23,6 @@ endif
> >>  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 8455994..e86911f 100644
> >>--- a/qemu-img.c
> >>+++ b/qemu-img.c
> >>@@ -30,6 +30,7 @@
> >>  #include "qemu/osdep.h"
> >>  #include "sysemu/sysemu.h"
> >>  #include "block/block_int.h"
> >>+#include "block/blockjob.h"
> >>  #include "block/qapi.h"
> >>  #include <getopt.h>
> >>@@ -682,12 +683,37 @@ fail:
> >>      return ret;
> >>  }
> >>+static void dummy_block_job_cb(void *opaque, int ret)
> >>+{
> >>+}
> >Why don't we need to check the return value?
> 
> I didn't check it, because it was not called – which apparently
> didn't sound strange to me at all. I checked and the much more
> interesting fact is that I assumed block_job_complete() would
> actually complete the block job without any further need for
> aio_poll(); but it doesn't. I'll fix both things (calling aio_poll()
> until the CB is called and checking the return value).

There is a block_job_cancel_sync(). Should we add a new
block_job_complete_sync() instead of adding the polling logic to
qemu-img?

Kevin