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

[Max Reitz]

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