From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:36805)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1TrA2C-0002Iw-OU
	for qemu-devel@nongnu.org; Fri, 04 Jan 2013 11:22:17 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1TrA2B-0001As-JK
	for qemu-devel@nongnu.org; Fri, 04 Jan 2013 11:22:16 -0500
Received: from mail-bk0-f46.google.com ([209.85.214.46]:35002)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1TrA2B-0001Aj-Bm
	for qemu-devel@nongnu.org; Fri, 04 Jan 2013 11:22:15 -0500
Received: by mail-bk0-f46.google.com with SMTP id q16so7439408bkw.19
	for <qemu-devel@nongnu.org>; Fri, 04 Jan 2013 08:22:14 -0800 (PST)
Date: Fri, 4 Jan 2013 17:22:12 +0100
From: Stefan Hajnoczi <stefanha@gmail.com>
Message-ID: <20130104162212.GE6310@stefanha-thinkpad.hitronhub.home>
References: <1355725509-5429-1-git-send-email-xiawenc@linux.vnet.ibm.com>
	<1355725509-5429-6-git-send-email-xiawenc@linux.vnet.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1355725509-5429-6-git-send-email-xiawenc@linux.vnet.ibm.com>
Subject: Re: [Qemu-devel] [PATCH 5/6] snapshot: qmp interface
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Cc: kwolf@redhat.com, pbonzini@redhat.com, aliguori@us.ibm.com, qemu-devel@nongnu.org, blauwirbel@gmail.com

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