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

[Fam Zheng <famz@redhat.com>]

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.

> >+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