On Wed, 12/17 10:36, Markus Armbruster wrote: > Fam Zheng <f...@redhat.com> writes: > > > Similar to drive-backup, but this command uses a device id as target > > instead of creating/opening an image file. > > > > Also add blocker on target bs, since the target is also a named device > > now. > > > > Add check and report error for bs == target which became possible but is > > an illegal case with introduction of blockdev-backup. > > > > Signed-off-by: Fam Zheng <f...@redhat.com> > > --- > > block/backup.c | 28 +++++++++++++++++++++++++++ > > blockdev.c | 47 +++++++++++++++++++++++++++++++++++++++++++++ > > qapi/block-core.json | 54 > > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > qmp-commands.hx | 44 ++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 173 insertions(+) > > > > diff --git a/block/backup.c b/block/backup.c > > index 792e655..b944dd4 100644 > > --- a/block/backup.c > > +++ b/block/backup.c > > @@ -360,6 +360,7 @@ static void coroutine_fn backup_run(void *opaque) > > hbitmap_free(job->bitmap); > > > > bdrv_iostatus_disable(target); > > + bdrv_op_unblock_all(target, job->common.blocker); > > > > data = g_malloc(sizeof(*data)); > > data->ret = ret; > > @@ -379,6 +380,11 @@ void backup_start(BlockDriverState *bs, > > BlockDriverState *target, > > assert(target); > > assert(cb); > > > > + if (bs == target) { > > + error_setg(errp, "Source and target cannot be the same"); > > + return; > > + } > > + > > if ((on_source_error == BLOCKDEV_ON_ERROR_STOP || > > on_source_error == BLOCKDEV_ON_ERROR_ENOSPC) && > > !bdrv_iostatus_is_enabled(bs)) { > > @@ -386,6 +392,26 @@ void backup_start(BlockDriverState *bs, > > BlockDriverState *target, > > return; > > } > > > > + if (!bdrv_is_inserted(bs)) { > > + error_setg(errp, "Devie is not inserted: %s", > > s/Devie/Device/
OK. > > > + bdrv_get_device_name(bs)); > > + return; > > + } > > + > > + if (!bdrv_is_inserted(target)) { > > + error_setg(errp, "Devie is not inserted: %s", > > Likewise. OK. > > > + bdrv_get_device_name(target)); > > + return; > > + } > > + > > + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) { > > + return; > > + } > > + > > + if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_BACKUP_TARGET, errp)) { > > + return; > > + } > > + > > len = bdrv_getlength(bs); > > if (len < 0) { > > error_setg_errno(errp, -len, "unable to get length for '%s'", > > @@ -399,6 +425,8 @@ void backup_start(BlockDriverState *bs, > > BlockDriverState *target, > > return; > > } > > > > + bdrv_op_block_all(target, job->common.blocker); > > + > > job->on_source_error = on_source_error; > > job->on_target_error = on_target_error; > > job->target = target; > > diff --git a/blockdev.c b/blockdev.c > > index 57910b8..2e5068c 100644 > > --- a/blockdev.c > > +++ b/blockdev.c > > @@ -2156,6 +2156,8 @@ void qmp_drive_backup(const char *device, const char > > *target, > > aio_context = bdrv_get_aio_context(bs); > > aio_context_acquire(aio_context); > > > > + /* Although backup_run has this check too, we need to use bs->drv > > below, so > > + * do an early check redundantly. */ > > if (!bdrv_is_inserted(bs)) { > > error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); > > goto out; > > @@ -2172,6 +2174,7 @@ void qmp_drive_backup(const char *device, const char > > *target, > > } > > } > > > > + /* Early check to avoid creating target */ > > if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) { > > goto out; > > } > > @@ -2239,6 +2242,50 @@ BlockDeviceInfoList > > *qmp_query_named_block_nodes(Error **errp) > > return bdrv_named_nodes_list(); > > } > > > > +void qmp_blockdev_backup(const char *device, const char *target, > > + enum MirrorSyncMode sync, > > + bool has_speed, int64_t speed, > > + bool has_on_source_error, > > + BlockdevOnError on_source_error, > > + bool has_on_target_error, > > + BlockdevOnError on_target_error, > > + Error **errp) > > +{ > > + BlockDriverState *bs; > > + BlockDriverState *target_bs; > > + Error *local_err = NULL; > > + > > + if (!has_speed) { > > + speed = 0; > > + } > > + if (!has_on_source_error) { > > + on_source_error = BLOCKDEV_ON_ERROR_REPORT; > > + } > > + if (!has_on_target_error) { > > + on_target_error = BLOCKDEV_ON_ERROR_REPORT; > > + } > > + > > + bs = bdrv_find(device); > > + 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); > > + 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); > > + error_propagate(errp, local_err); > > + } > > +} > > + > > #define DEFAULT_MIRROR_BUF_SIZE (10 << 20) > > > > void qmp_drive_mirror(const char *device, const char *target, > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > index 77a0cfb..bc02bd7 100644 > > --- a/qapi/block-core.json > > +++ b/qapi/block-core.json > > @@ -676,6 +676,41 @@ > > '*on-target-error': 'BlockdevOnError' } } > > > > ## > > +# @BlockdevBackup > > +# > > +# @device: the name of the device which should be copied. > > +# > > +# @target: the name of the backup target device. > > +# > > +# @sync: what parts of the disk image should be copied to the destination > > +# (all the disk, only the sectors allocated in the topmost image, or > > +# only new I/O). > > +# > > +# @speed: #optional the maximum speed, in bytes per second. The default is > > 0, > > +# for unlimited. > > +# > > +# @on-source-error: #optional 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 (see BlockInfo). > > +# > > +# @on-target-error: #optional the action to take on an error on the target, > > +# default 'report' (no limitations, since this applies to > > +# a different block device than @device). > > +# > > +# Note that @on-source-error and @on-target-error only affect background > > I/O. > > +# If an error occurs during a guest write request, the device's > > rerror/werror > > +# actions will be used. > > +# > > +# Since: 2.3 > > +## > > +{ 'type': 'BlockdevBackup', > > + 'data': { 'device': 'str', 'target': 'str', > > + 'sync': 'MirrorSyncMode', > > + '*speed': 'int', > > + '*on-source-error': 'BlockdevOnError', > > + '*on-target-error': 'BlockdevOnError' } } > > + > > +## > > # @blockdev-snapshot-sync > > # > > # Generates a synchronous snapshot of a block device. > > @@ -795,6 +830,25 @@ > > { 'command': 'drive-backup', 'data': 'DriveBackup' } > > > > ## > > +# @blockdev-backup > > +# > > +# Start a point-in-time copy of a block device to a new destination. The > > +# status of ongoing blockdev-backup operations can be checked with > > +# query-block-jobs where the BlockJobInfo.type field has the value > > 'backup'. > > +# The operation can be stopped before it has completed using the > > +# block-job-cancel command. > > +# > > +# For the arguments, see the documentation of BlockdevBackup. > > +# > > +# Returns: Nothing on success. > > +# If @device or @target is not a valid block device, > > DeviceNotFound. > > Why do you need an error classes other than GenericError here? > Because this is what other block job commands do, and I followed the style. > > +# > > +# Since 2.3 > > +## > > +{ 'command': 'blockdev-backup', 'data': 'BlockdevBackup' } > > + > > + > > +## > > # @query-named-block-nodes > > # > > # Get the named block driver list > > diff --git a/qmp-commands.hx b/qmp-commands.hx > > index 1abd619..518df0c 100644 > > --- a/qmp-commands.hx > > +++ b/qmp-commands.hx > > @@ -1094,6 +1094,50 @@ Example: > > "sync": "full", > > "target": "backup.img" } } > > <- { "return": {} } > > + > > +EQMP > > + > > + { > > + .name = "blockdev-backup", > > + .args_type = "sync:s,device:B,target:B,speed:i?," > > + "on-source-error:s?,on-target-error:s?", > > + .mhandler.cmd_new = qmp_marshal_input_blockdev_backup, > > + }, > > + > > +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) > > Still copied from drive-backup without change. I don't think it's > correct here. Either fix it, or explain to me why I'm wrong :) Overlooked by my eyes, will fix in v5 (I posted v4 earlier in Dec and got comments from Max that are pending to be addressed). Thanks for reviewing! Fam > > > +- "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" } } > > +<- { "return": {} } > > + > > EQMP > > > > {