On Fri, 12/19 09:20, Markus Armbruster wrote:
> Fam Zheng <f...@redhat.com> writes:
>
> > 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.
> [...]
> >> > 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;
> >> > + }
>
> New use of a QERR_ macro. See below.
>
> >> > +
> >> > + 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.
>
> Following existing practice is a good idea, except when you're following
> bad examples.
>
> When we scrapped the misguided "rich error" idea, we undid some, but not
> all of its damage (series around commit de253f1). In particular, we got
> rid of most, but not all ErrorClass values. We had to keep a few to
> avoid breaking existing QMP clients. They've been sneaking into new
> code ever since, hiding in their QERR_ macros.
>
> Quoting the Code Transitions Wiki page:
>
> * Avoid ErrorClass values other than ERROR_CLASS_GENERIC_ERROR unless
> you have a specific reason. Prefer error_setg() & friends over
> error_set() & friends.
>
> * The QError macros QERR_ are a hack to help with the transition to the
> current error.h API (human-readable message rather than JSON argument,
> see commit df1e608). Avoid them in new code, use simple message
> strings instead.
>
> We're going to deprecate a number of block-related commands for other
> reasons. I want their replacements avoid ErrorClass values other than
> ERROR_CLASS_GENERIC_ERROR. Same for new commands.
>
> You don't have to respin for this, I can fix it up on top.
>
>
> [*] http://wiki.qemu.org/CodeTransitions#Error_reporting
OK, thanks for explaining and taking care of it. I'll follow your future
patches.
Fam
