From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38679) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TqPgU-0001ZP-RX for qemu-devel@nongnu.org; Wed, 02 Jan 2013 09:52:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TqPgQ-0007us-GG for qemu-devel@nongnu.org; Wed, 02 Jan 2013 09:52:46 -0500 Received: from mx1.redhat.com ([209.132.183.28]:44072) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TqPgQ-0007ub-5t for qemu-devel@nongnu.org; Wed, 02 Jan 2013 09:52:42 -0500 Message-ID: <50E449B4.1000803@redhat.com> Date: Wed, 02 Jan 2013 07:52:36 -0700 From: Eric Blake MIME-Version: 1.0 References: <1355725509-5429-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1355725509-5429-6-git-send-email-xiawenc@linux.vnet.ibm.com> In-Reply-To: <1355725509-5429-6-git-send-email-xiawenc@linux.vnet.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enig11D724B8538287C8C9E35F5F" Subject: Re: [Qemu-devel] [PATCH 5/6] snapshot: qmp interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia Cc: kwolf@redhat.com, aliguori@us.ibm.com, stefanha@gmail.com, qemu-devel@nongnu.org, blauwirbel@gmail.com, pbonzini@redhat.com This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig11D724B8538287C8C9E35F5F Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 12/16/2012 11:25 PM, Wenchao Xia wrote: > This patch changes the implemtion of external block snapshot s/implemtion/implementation/ > to use internal unified interface, now qmp handler just do s/do/does/ > a translation of request and submit. > Also internal block snapshot qmp interface was added. > Now add external snapshot, add/delete internal snapshot > can be started in their own qmp interface or a group of > BlockAction in qmp transaction interface. >=20 > Signed-off-by: Wenchao Xia > --- > +++ b/qapi-schema.json I didn't look at the code, because I want to make sure we get the interface right, first. > @@ -1458,17 +1458,36 @@ > { 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' = }} > =20 > ## > +# @SnapshotType > +# > +# An enumeration that tells QEMU what type of snapshot to access. > +# > +# @internal: QEMU should use internal snapshot in format such as qcow2= =2E > +# > +# @external: QEMU should use backing file chain. > +# > +# Since: 1.4. > +## > +{ 'enum': 'SnapshotType' > + 'data': [ 'internal', 'external' ] } > + > +## > # @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 s= napshot > +# record. In external snapshot case, qemu will skip create = new image > +# file, In internal snapshot case qemu will try use the exi= sting 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 crea= te a new > +# snapshot record in internal snapshot case which wil= l > +# overwrite internal snapshot record if it already ex= ist. 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. > # > -# 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 crea= ted. > +# @snapshot-file: the target name of the snapshot. In external case, i= t 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. > +# 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 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). > # > # @format: #optional the format of the snapshot image, default is 'qco= w2'. > # > -# @mode: #optional whether and how QEMU should create a new image, def= ault is > -# 'absolute-paths'. > +# @mode: #optional whether QEMU should create a new snapshot or use ex= isting > +# 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? > +# > +# @type: #optional internal snapshot or external, default is 'external= '. Mention that this field is new since 1.4. > +# > ## > { '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, i= t is > +# the file's name to be merged, In internal case, it i= s the > +# internal snapshot record's name. What happens if there is no record name (since the qcow2 file does not require one)? > +# > +# @type: #optional internal snapshot or external, default is > +# 'external', note that delete 'external' snapshot is not suppo= rted > +# 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? > +## > +{ 'type': 'BlockdevSnapshotDelete', > + 'data': { 'device': 'str', 'snapshot-file': 'str', > + '*type': 'SnapshotType'} } > =20 > ## > # @BlockdevAction > @@ -1498,6 +1540,7 @@ > { 'union': 'BlockdevAction', > 'data': { > 'blockdev-snapshot-sync': 'BlockdevSnapshot', > + 'blockdev-snapshot-delete-sync': 'BlockdevSnapshotDelete', > } } > =20 > ## > @@ -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 exi= sting > -# file/device. If does not exist, a new file will be c= reated. > +# @snapshot-file: the target name of the snapshot. In external case, i= t 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 c= reated in > +# the existing file/device. If does not exist, a new f= ile will > +# be created. In internal case, it is the internal sna= pshot > +# record's name, if it is 'blank' name will be generat= ed > +# according to time. Again, special casing the empty string seems awkward; making the field optional for the internal case might make more sense. > # > # @format: #optional the format of the snapshot image, default is 'qco= w2'. Is this field compatible with internal snapshots? > # > # @mode: #optional whether and how QEMU should create a new image, def= ault is > # 'absolute-paths'. > # > +# @type: #optional internal snapshot or external, default is > +# 'external'. Mention that this field was added in 1.4. > +# > # 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. 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. > +# > +# @device: the name of the device to delete the snapshot from. > +# > +# @snapshot-file: the target name of the snapshot. In external case, i= t is > +# the file's name to be merged, In internal case, it i= s the > +# internal snapshot record's name. > +# > +# @type: #optional internal snapshot or external, default is > +# 'external', note that delete 'external' snapshot is not suppo= rted > +# 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'} } > =20 > ## > # @human-monitor-command: >=20 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enig11D724B8538287C8C9E35F5F Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with undefined - http://www.enigmail.net/ iQEcBAEBCAAGBQJQ5Em0AAoJEKeha0olJ0NqSGgH/0ofaG+M15Uemoi2uxKhNSB0 9fuz/A2Uy7+ju96Pa8mGSKekLac3BRxzbwRJ0Jb89VowGxgHmopwkjQ7/H1UF4bp k/qbO6vRyl8xP7xCgZnM/BhdxiFgmZqnMTFgzik1Rzo5y5zIsqkQja2fhJlAbrDu Ifc/cEWIfEATXysfylFgF3+U3smZ64RgPV7aw/IOf4DWiFqTHwu4+grvDDVmyrw+ 7W+tFHZvzjG0Hl7b0NeWBw6A7e6i1qnIku/NXOc9zfc5PecZ+9UmNDoGhBRsC5V+ HekTG9CU3vtWdb+XtSpD5VubmpVRYLN4Y4fF7s6O82jovrNJ7ZsTjY4CJMiNxfY= =UVct -----END PGP SIGNATURE----- --------------enig11D724B8538287C8C9E35F5F--