|
From: | Max Reitz |
Subject: | Re: [Qemu-devel] [PATCH v4 1/3] qmp: Add command 'blockdev-backup' |
Date: | Fri, 05 Dec 2014 10:10:17 +0100 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 |
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
[Prev in Thread] | Current Thread | [Next in Thread] |