qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH v2 3/6] qemu-img: Implement commit like QMP
Date: Sat, 12 Apr 2014 19:04:48 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0

On 11.04.2014 14:54, Kevin Wolf wrote:
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?

I don't see the immediate need for it, as we still need the polling logic while running the block job. However, if there is a block_job_cancel_sync() (as you said), it would make sense to introduce the same for "complete" and reuse it here. I'll send a v4, then.

Max



reply via email to

[Prev in Thread] Current Thread [Next in Thread]