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

[Markus Armbruster]

Fam Zheng <address@hidden> writes:

> On Wed, 12/17 10:36, Markus Armbruster wrote:
>> Fam Zheng <address@hidden> 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