Re: [Qemu-devel] [PATCH v4 1/3] qmp: Add command 'blockdev-backup'

[Max Reitz <mreitz@redhat.com>]

On 2014-12-05 at 07:12, Fam Zheng wrote:
> On Thu, 12/04 14:43, Max Reitz wrote:
>>> +    if (!bs) {
>>> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
>>> +        return;
>>> +    }
>>> +
>>> +    target_bs = bdrv_find(target);
>>> +    if (!target_bs) {
>>> +        error_set(errp, QERR_DEVICE_NOT_FOUND, target);
>>> +        return;
>>> +    }
>>> +
>>> +    bdrv_ref(target_bs);
>>> +    bdrv_set_aio_context(target_bs, bdrv_get_aio_context(bs));
>> In the cover letter you said you were acquiring the AIO context but you're
>> not. Something like the aio_context_acquire() call in qmp_drive_backup()
>> seems missing.
> Yes. Adding.
>
>>> +    backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
>>> +                 block_job_cb, bs, &local_err);
>>> +    if (local_err != NULL) {
>>> +        bdrv_unref(target_bs);
>> Hm, as far as I can see, backup_complete() is always run, regardless of the
>> operation status. backup_complete() in turn calls bdrv_unref(s->target), so
>> this seems unnecessary to me.
> In error case, backup_start does nothing. So we need to release for the
> bdrv_ref two lines above.

Ah, right, I forgot about early errors. local_err is not even set if the 
block job itself fails, because by the time the block job is started, 
backup_start() will have already returned. My fault.

Max

>>> +SQMP
>>> +blockdev-backup
>>> +------------
>>> +
>>> +The device version of drive-backup: this command takes an existing named device
>>> +as backup target.
>>> +
>>> +Arguments:
>>> +
>>> +- "device": the name of the device which should be copied.
>>> +            (json-string)
>>> +- "target": the target of the new image. If the file exists, or if it is a
>>> +            device, the existing file/device will be used as the new
>>> +            destination.  If it does not exist, a new file will be created.
>>> +            (json-string)
>>> +- "sync": what parts of the disk image should be copied to the destination;
>>> +          possibilities include "full" for all the disk, "top" for only the
>>> +          sectors allocated in the topmost image, or "none" to only replicate
>>> +          new I/O (MirrorSyncMode).
>>> +- "speed": the maximum speed, in bytes per second (json-int, optional)
>>> +- "on-source-error": the action to take on an error on the source, default
>>> +                     'report'.  'stop' and 'enospc' can only be used
>>> +                     if the block device supports io-status.
>>> +                     (BlockdevOnError, optional)
>>> +- "on-target-error": the action to take on an error on the target, default
>>> +                     'report' (no limitations, since this applies to
>>> +                     a different block device than device).
>>> +                     (BlockdevOnError, optional)
>>> +
>>> +Example:
>>> +-> { "execute": "blockdev-backup", "arguments": { "device": "src-id",
>>> +                                                  "target": "tgt-id" } }
>> Isn't "sync" missing?
> Yes.
>
> Thanks,
> Fam