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

[John Snow]

On 7/5/19 10:14 AM, Markus Armbruster wrote:
> 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?
> 

Yes.

>>  #
>> -# @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?
> 

Yes.

>>  #
>>  # @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.
> 

OK.

>>  #
>>  # @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.
> 

Ah, sure.

>>  ##
>> +{ '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: