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

[Fam Zheng]

On Fri, 12/19 09:20, Markus Armbruster wrote:
> 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

OK, thanks for explaining and taking care of it. I'll follow your future
patches.

Fam