Re: [Qemu-devel] [PATCH v2 01/18] qapi/block-core: Introduce BackupCommon

[Markus Armbruster]

John Snow <address@hidden> writes:

> drive-backup and blockdev-backup have an awful lot of things in common
> that are the same. Let's fix that.
>
> I don't deduplicate 'target', because the semantics actually did change
> between each structure. Leave that one alone so it can be documented
> separately.
>
> Signed-off-by: John Snow <address@hidden>
> ---
>  qapi/block-core.json | 103 ++++++++++++++-----------------------------
>  1 file changed, 33 insertions(+), 70 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0d43d4f37c..7b23efcf13 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1315,32 +1315,23 @@
>    'data': { 'node': 'str', 'overlay': 'str' } }
>  
>  ##
> -# @DriveBackup:
> +# @BackupCommon:
>  #
>  # @job-id: identifier for the newly-created block job. If
>  #          omitted, the device name will be used. (Since 2.7)
>  #
>  # @device: the device name or node-name of a root node which should be 
> copied.
>  #
> -# @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.
> -#
> -# @format: the format of the new destination, default is to
> -#          probe if @mode is 'existing', else the format of the source
> -#
>  # @sync: what parts of the disk image should be copied to the destination
>  #        (all the disk, only the sectors allocated in the topmost image, 
> from a
>  #        dirty bitmap, or only new I/O).

This is DriveBackup's wording.  Blockdev lacks "from a dirty bitmap, ".
Is this a doc fix?

>  #
> -# @mode: whether and how QEMU should create a new image, default is
> -#        'absolute-paths'.
> -#
> -# @speed: the maximum speed, in bytes per second
> +# @speed: the maximum speed, in bytes per second. The default is 0,
> +#         for unlimited.

This is Blockdev's wording.  DriveBackup lacks "the default is 0, for
unlimited."  Is this a doc fix?

>  #
>  # @bitmap: the name of dirty bitmap if sync is "incremental".
>  #          Must be present if sync is "incremental", must NOT be present
> -#          otherwise. (Since 2.4)
> +#          otherwise. (Since 2.4 (Drive), 3.1 (Blockdev))

I second Max's request to say (drive-backup) and (blockdev-backup),
strongly.

>  #
>  # @compress: true to compress data, if the target format supports it.
>  #            (default: false) (since 2.8)
> @@ -1372,73 +1363,45 @@
>  #
>  # Since: 1.6

BackupCommon is actually since 4.2.

The type doesn't appear in the external interface (only its extensions
DriveBackup and BlockdevBackup do), so documenting "since" is actually
pointless.  However, we blindly document "since" for *everything*,
simply because we don't want to waste time on deciding whether we have
to.

>  ##
> +{ 'struct': 'BackupCommon',
> +  'data': { '*job-id': 'str', 'device': 'str',
> +            'sync': 'MirrorSyncMode', '*speed': 'int',
> +            '*bitmap': 'str', '*compress': 'bool',
> +            '*on-source-error': 'BlockdevOnError',
> +            '*on-target-error': 'BlockdevOnError',
> +            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
> +
> +##
> +# @DriveBackup:
> +#
> +# @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.
> +#
> +# @format: the format of the new destination, default is to
> +#          probe if @mode is 'existing', else the format of the source
> +#
> +# @mode: whether and how QEMU should create a new image, default is
> +#        'absolute-paths'.
> +#
> +# Since: 1.6
> +##
>  { 'struct': 'DriveBackup',
> -  'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
> -            '*format': 'str', 'sync': 'MirrorSyncMode',
> -            '*mode': 'NewImageMode', '*speed': 'int',
> -            '*bitmap': 'str', '*compress': 'bool',
> -            '*on-source-error': 'BlockdevOnError',
> -            '*on-target-error': 'BlockdevOnError',
> -            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
> +  'base': 'BackupCommon',
> +  'data': { 'target': 'str',
> +            '*format': 'str',
> +            '*mode': 'NewImageMode' } }
>  
>  ##
>  # @BlockdevBackup:
>  #
> -# @job-id: identifier for the newly-created block job. If
> -#          omitted, the device name will be used. (Since 2.7)
> -#
> -# @device: the device name or node-name of a root node which should be 
> copied.
> -#
>  # @target: the device name or node-name of the backup target node.
>  #
> -# @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: the maximum speed, in bytes per second. The default is 0,
> -#         for unlimited.
> -#
> -# @bitmap: the name of dirty bitmap if sync is "incremental".
> -#          Must be present if sync is "incremental", must NOT be present
> -#          otherwise. (Since 3.1)
> -#
> -# @compress: true to compress data, if the target format supports it.
> -#            (default: false) (since 2.8)
> -#
> -# @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 (see BlockInfo).
> -#
> -# @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).
> -#
> -# @auto-finalize: When false, this job will wait in a PENDING state after it 
> has
> -#                 finished its work, waiting for @block-job-finalize before
> -#                 making any block graph changes.
> -#                 When true, this job will automatically
> -#                 perform its abort or commit actions.
> -#                 Defaults to true. (Since 2.12)
> -#
> -# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
> -#                has completely ceased all work, and awaits 
> @block-job-dismiss.
> -#                When true, this job will automatically disappear from the 
> query
> -#                list without user intervention.
> -#                Defaults to true. (Since 2.12)
> -#
> -# Note: @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
>  ##
>  { 'struct': 'BlockdevBackup',
> -  'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
> -            'sync': 'MirrorSyncMode', '*speed': 'int',
> -            '*bitmap': 'str', '*compress': 'bool',
> -            '*on-source-error': 'BlockdevOnError',
> -            '*on-target-error': 'BlockdevOnError',
> -            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
> +  'base': 'BackupCommon',
> +  'data': { 'target': 'str' } }
>  
>  ##
>  # @blockdev-snapshot-sync: