[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 1/3] qmp: Add command 'blockdev-backup'
From: |
Fam Zheng |
Subject: |
Re: [Qemu-devel] [PATCH v4 1/3] qmp: Add command 'blockdev-backup' |
Date: |
Fri, 5 Dec 2014 14:12:49 +0800 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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