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

[Stefan Hajnoczi]

On Mon, Dec 17, 2012 at 02:25:08PM +0800, Wenchao Xia wrote:
> @@ -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
> +#                 case, it is the internal snapshot record's name and if it 
> is
> +#                 'blank' name will be generated according to time.
>  #
>  # @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'.
> +#
> +# @type: #optional internal snapshot or external, default is 'external'.
> +#
>  ##
>  { 'type': 'BlockdevSnapshot',
>    'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
> -            '*mode': 'NewImageMode' } }
> +            '*mode': 'NewImageMode', '*type': 'SnapshotType'} }

It feels cleaner to define a new BlockdevInternalSnapshot type instead
of overloading the field semantics.  The naming doesn't fit for internal
snapshots (e.g. "snapshot-file", "absolute-paths", etc).

Treat internal snapshots as a new transaction action.  That way we can
cleanly add internal snapshot specific fields in the future.  The
documentation will be clearer because users won't have to consider the
"if type is 'external', then ..., otherwise ...".

Stefan