Re: [Qemu-devel] [PATCH v13 08/14] qemu-img: Implement commit like QMP

[Max Reitz <mreitz@redhat.com>]

On 2014-10-23 at 14:40, Kevin Wolf wrote:
> Am 23.10.2014 um 14:35 hat Max Reitz geschrieben:
>> On 2014-10-23 at 13:59, Kevin Wolf wrote:
>>> 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 <mreitz@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?
>> I remember you telling that I puzzled this code together until it
>> worked. At one point in time, it didn't work without this. It works
>> now. I'm going to trust you.
> This was an honest question. It's probably risky to trust that if I
> don't understand something, the opposite is true...

I remember having had trouble with the block job just pausing and then 
qemu-img would sleep indefinitely. Maybe I'm mistaken. Maybe it actually 
was necessary back in April.

Right now I don't see a way how the block job should be paused aside 
from errors and manual QMP intervention.

Max