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

[Max Reitz <mreitz@redhat.com>]

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.

> While trying out what would happen with failing requests (both for
> checking this and what error message I would get), my image got
> corrupted (as I told you on IRC):
>
> $ qemu-img create -f qcow2 -b /tmp/backing.qcow2 /tmp/test.qcow2 64M
> $ qemu-img commit blkdebug::/tmp/test.qcow2
> qemu-img: Could not empty blkdebug::/tmp/test.qcow2: Operation not supported
> $ qemu-io -c 'write 0 1M' /tmp/test.qcow2
> qcow2: Marking image as corrupt: Preventing invalid write on metadata
> (overlaps with active L1 table); further corruption events will be
> suppressed
> write failed: Input/output error

As discussed on IRC, this is due to qcow2 marking every image clean when 
it's closed. Setting bs->drv to NULL solves this issue.

>> +    } while (!job->ready);
>> +
>> +    block_job_complete_sync(job, errp);
>> +}
>> +
>>   static int img_commit(int argc, char **argv)
>>   {
>>       int c, ret, flags;
>>       const char *filename, *fmt, *cache;
>>       BlockBackend *blk;
>> -    BlockDriverState *bs;
>> +    BlockDriverState *bs, *base_bs;
>>       bool quiet = false;
>> +    Error *local_err = NULL;
>> +    CommonBlockJobCBInfo cbi;
>>   
>>       fmt = NULL;
>>       cache = BDRV_DEFAULT_CACHE;
>> @@ -764,29 +799,38 @@ static int img_commit(int argc, char **argv)
>>       }
>>       bs = blk_bs(blk);
>>   
>> -    ret = bdrv_commit(bs);
>> -    switch(ret) {
>> -    case 0:
>> -        qprintf(quiet, "Image committed.\n");
>> -        break;
>> -    case -ENOENT:
>> -        error_report("No disk inserted");
>> -        break;
>> -    case -EACCES:
>> -        error_report("Image is read-only");
>> -        break;
>> -    case -ENOTSUP:
>> -        error_report("Image is already committed");
>> -        break;
>> -    default:
>> -        error_report("Error while committing image");
>> -        break;
>> +    /* This is different from QMP, which by default uses the deepest file in the
>> +     * backing chain (i.e., the very base); however, the traditional behavior of
>> +     * qemu-img commit is using the immediate backing file. */
>> +    base_bs = bs->backing_hd;
>> +    if (!base_bs) {
>> +        error_set(&local_err, QERR_BASE_NOT_FOUND, "NULL");
> $ qemu-img create -f qcow2 /tmp/test.qcow2 64M
> Formatting '/tmp/test.qcow2', fmt=qcow2 size=67108864 encryption=off cluster_size=65536 lazy_refcounts=off
> $ qemu-img commit /tmp/test.qcow2
> qemu-img: Base 'NULL' not found
>
> Do we expect any user to understand what we want to tell them? This
> should clearly be something along the lines of error_setg(&local_err,
> "Image doesn't have a backing file"). (Before this patch, it said
> "Image is already committed", which isn't great either, but not as bad
> as the new message)

Yes, will fix.

Max