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

[Wenchao Xia]

  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