Re: [Qemu-devel] [PATCH 5/6] snapshot: qmp interface

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

   Thanks for reviewing, I'll correct the spelling.

>> +
>> +##
>>   # @NewImageMode
>>   #
>>   # An enumeration that tells QEMU how to set the backing file path in
>> -# a new image file.
>> +# a new image file, or how to use internal snapshot record.
>>   #
>> -# @existing: QEMU should look for an existing image file.
>> +# @existing: QEMU should look for an existing image file or internal snapshot
>> +#            record. In external snapshot case, qemu will skip create new image
>> +#            file, In internal snapshot case qemu will try use the existing
>
> s/In/in/
>
>> +#            one. if not found operation would fail.
>
> s/. if/. If/; s/would/will/
>
>>   #
>> -# @absolute-paths: QEMU should create a new image with absolute paths
>> -# for the backing file.
>> +# @absolute-paths: QEMU should create a new image with absolute paths for
>> +#                  the backing file in external snapshot case, or create a new
>> +#                  snapshot record in internal snapshot case which will
>> +#                  overwrite internal snapshot record if it already exist.
>
> Doesn't quite make sense - internal snapshots don't record a path, so
> why is absolute-paths the right mode for requesting the creation of a
> new snapshot?   I think it would make more sense if you add a new mode,
> and then declare that absolute-paths is invalid for internal snapshots,
> and that the new mode is invalid for external snapshots.
>
   OK,

>>   #
>> -# Since: 1.1
>> +# Since: 1.1, internal support since 1.4.
>>   ##
>>   { 'enum': 'NewImageMode'
>>     'data': [ 'existing', 'absolute-paths' ] }
>> @@ -1478,16 +1497,39 @@
>>   #
>>   # @device:  the name of the device to generate the snapshot from.
>>   #
>> -# @snapshot-file: the target of the new image. A new file will be created.
>> +# @snapshot-file: the target name of the snapshot. In external case, it is
>> +#                 the new file's name, A new file will be created. In internal
>
> s/A/a/
>
> and a new file is only created according to mode.
>
   OK.

>> +#                 case, it is the internal snapshot record's name and if it is
>> +#                 'blank' name will be generated according to time.
>
> Ugg.  Passing an empty string for snapshot-file as a special case seems
> awkward; it might be better to make it an optional argument via
> '*snapshot-file', where the argument is mandatory for external, but
   I think *snapshot-file is great, but it will change the interface
which already exist triggering compatialbility issue, is this allowed?

> omitting the argument on internal allows the fallback naming.  Or why do
> you even need to worry about fallback naming?  Requiring the user to
> always provide a record name may be easier to support (certainly fewer
> corner cases to worry about).
>
   If cost is low, I think providing it is not bad, and thus hmp/qmp
can have same capability.

>>   #
>>   # @format: #optional the format of the snapshot image, default is 'qcow2'.
>>   #
>> -# @mode: #optional whether and how QEMU should create a new image, default is
>> -#        'absolute-paths'.
>> +# @mode: #optional whether QEMU should create a new snapshot or use existing
>> +#        one, default is 'absolute-paths'.
>
> Does this default still make sense for internal snapshots, or do you
> need to document that the default mode differs depending on the type of
> snapshot being taken?
>
   I'll add another optional argument and keeps mode only for external
snapshot.

>> +#
>> +# @type: #optional internal snapshot or external, default is 'external'.
>
> Mention that this field is new since 1.4.
>
   OK.

>> +#
>>   ##
>>   { 'type': 'BlockdevSnapshot',
>>     'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
>> -            '*mode': 'NewImageMode' } }
>> +            '*mode': 'NewImageMode', '*type': 'SnapshotType'} }
>> +
>> +##
>> +# @BlockdevSnapshotDelete
>> +#
>> +# @device:  the name of the device to delete the snapshot from.
>> +#
>> +# @snapshot-file: the target name of the snapshot. In external case, it is
>> +#                 the file's name to be merged, In internal case, it is the
>> +#                 internal snapshot record's name.
>
> What happens if there is no record name (since the qcow2 file does not
> require one)?
   I think snapshot id should be provided then.

>> +#
>> +# @type: #optional internal snapshot or external, default is
>> +#        'external', note that delete 'external' snapshot is not supported
>> +#        now for that it is the same to commit it.
>
> If external is not supported, then why do you even need this parameter?
>   Besides, shouldn't it be pretty obvious from the snapshot-file argument
> whether the snapshot exists internally or externally, without needing
> the user to tell you that?
>
   I thought it is not easy to tell the difference just from file, could
u tip how?

>> +##
>> +{ 'type': 'BlockdevSnapshotDelete',
>> +  'data': { 'device': 'str', 'snapshot-file': 'str',
>> +            '*type': 'SnapshotType'} }
>>
>>   ##
>>   # @BlockdevAction
>> @@ -1498,6 +1540,7 @@
>>   { 'union': 'BlockdevAction',
>>     'data': {
>>          'blockdev-snapshot-sync': 'BlockdevSnapshot',
>> +       'blockdev-snapshot-delete-sync': 'BlockdevSnapshotDelete',
>>      } }
>>
>>   ##
>> @@ -1530,23 +1573,54 @@
>>   #
>>   # @device:  the name of the device to generate the snapshot from.
>>   #
>> -# @snapshot-file: the target of the new image. If the file exists, or if it
>> -#                 is a device, the snapshot will be created in the existing
>> -#                 file/device. If does not exist, a new file will be created.
>> +# @snapshot-file: the target name of the snapshot. In external case, it is
>> +#                 the new file's name, A new file will be created. If the file
>> +#                 exists, or if it is a device, the snapshot will be created in
>> +#                 the existing file/device. If does not exist, a new file will
>> +#                 be created. In internal case, it is the internal snapshot
>> +#                 record's name, if it is 'blank' name will be generated
>> +#                 according to time.
>
> Again, special casing the empty string seems awkward; making the field
> optional for the internal case might make more sense.
>
   OK, if API compatible issue is acceptable.


>>   #
>>   # @format: #optional the format of the snapshot image, default is 'qcow2'.
>
> Is this field compatible with internal snapshots?
>
   no, I'll doc it.

>>   #
>>   # @mode: #optional whether and how QEMU should create a new image, default is
>>   #        'absolute-paths'.
>>   #
>> +# @type: #optional internal snapshot or external, default is
>> +#        'external'.
>
> Mention that this field was added in 1.4.
>
   OK.

>> +#
>>   # Returns: nothing on success
>>   #          If @device is not a valid block device, DeviceNotFound
>>   #
>> -# Since 0.14.0
>> +# Since 0.14.0, internal snapshot supprt since 1.4.
>>   ##
>>   { 'command': 'blockdev-snapshot-sync',
>>     'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
>> -            '*mode': 'NewImageMode'} }
>> +            '*mode': 'NewImageMode', '*type': 'SnapshotType'} }
>> +
>> +##
>> +# @blockdev-snapshot-delete-sync
>> +#
>> +# Delete a synchronous snapshot of a block device.
>
> Wrong.  Rather, this is a synchronous operation that deletes a snapshot,
> regardless of whether the snapshot was created synchronously or
> asynchronously.
>
   OK, the doc will be changed to tip better.

> Remember, the original goal was to come up with a way to take snapshots
> in a way that didn't require blocking until the operation was done; and
> once such an interface is present, then that becomes a new operation
> blockdev-snapshot-async (or more than one command, in order to drive the
> sequence of operations needed).  blockdev-snapshot-sync would then be
> rewritten as a wrapper that calls the underlying sequence of operations
> in one command, blocking until complete.
>
> But for deletion, we might as well do it right from the beginning.  I
> wonder if you can even design things to just have
> 'blockdev-snapshot-delete' without needing to distinguish between a sync
> or async operation.
>
   There is already API as blockdev-snapshot-sync, this will break
the mirror, do you think it is OK? Actually I think it is OK
to redesign API including old one as blockdev-snapshot,
  blockdev-snapshot-delete, but again it brings compatible issue.
Or another solution which keep API unchanged as:
blockdev-snapshot-sync
blockdev-snapshot-delete-sync
blockdev-snapshot-async
blockdev-snapshot-delete-async

>> +#
>> +# @device:  the name of the device to delete the snapshot from.
>> +#
>> +# @snapshot-file: the target name of the snapshot. In external case, it is
>> +#                 the file's name to be merged, In internal case, it is the
>> +#                 internal snapshot record's name.
>> +#
>> +# @type: #optional internal snapshot or external, default is
>> +#        'external', note that delete 'external' snapshot is not supported
>> +#        now for that it is the same to commit it.
>
> Again, do you really need this field, or can it be inferred from
> snapshot-file?
>
>> +#
>> +# Returns: nothing on success
>> +#          If @device is not a valid block device, DeviceNotFound
>> +#
>> +# Since 1.4
>> +##
>> +{ 'command': 'blockdev-snapshot-delete-sync',
>> +  'data': { 'device': 'str', 'snapshot-file': 'str',
>> +            '*type': 'SnapshotType'} }
>>
>>   ##
>>   # @human-monitor-command:
>>
>


-- 
Best Regards

Wenchao Xia