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

[Max Reitz <mreitz@redhat.com>]

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

Max

> Kevin