From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41387) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UnnMR-0001Gp-Sn for qemu-devel@nongnu.org; Sat, 15 Jun 2013 06:05:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UnnMQ-00036Q-CY for qemu-devel@nongnu.org; Sat, 15 Jun 2013 06:05:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46380) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UnnMQ-00036L-46 for qemu-devel@nongnu.org; Sat, 15 Jun 2013 06:05:30 -0400 Message-ID: <51BC367D.6030500@redhat.com> Date: Sat, 15 Jun 2013 10:40:13 +0100 From: Eric Blake MIME-Version: 1.0 References: <1371209999-15579-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1371209999-15579-8-git-send-email-xiawenc@linux.vnet.ibm.com> In-Reply-To: <1371209999-15579-8-git-send-email-xiawenc@linux.vnet.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2QRCWSLHAIEQHMBPRXQOB" Subject: Re: [Qemu-devel] [PATCH V2 07/12] qmp: add internal snapshot support in qmp_transaction List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia Cc: kwolf@redhat.com, phrdina@redhat.com, famz@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, stefanha@redhat.com, pbonzini@redhat.com, dietmar@proxmox.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2QRCWSLHAIEQHMBPRXQOB Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 06/14/2013 12:39 PM, Wenchao Xia wrote: > Unlike savevm, the qmp_transaction interface will not generate > snapshot name automatically, saving trouble to return information > of the new created snapshot. The snapshot name should not mess up > with snapshot ID, there is a check for it. >=20 > Although qcow2 support storing multiple snapshots with same name > but different ID, here it will fail when an snapshot with that name > already exist before the operation. Format such as rbd do not support > ID at all, and in most case, it means trouble to user when he faces > multiple snapshots with same name, so ban that case. >=20 > Snapshot ID can't be specified in this interface. >=20 > Signed-off-by: Wenchao Xia > --- > blockdev.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++= ++++++++ > qapi-schema.json | 16 +++++++ > qmp-commands.hx | 32 +++++++++++--- > 3 files changed, 159 insertions(+), 7 deletions(-) > +++ b/qapi-schema.json > @@ -1613,6 +1613,21 @@ > { 'type': 'BlockdevSnapshot', > 'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',= > '*mode': 'NewImageMode' } } > +## > +# @BlockdevSnapshotInternal > +# > +# @device: the name of the device to generate the snapshot from > +# > +# @name: the name of the internal snapshot to be created > +# > +# Notes: In transaction, if any snapshot matching @name exists, the op= eration > +# will fail. If the name is a numeric string, it will also fail= =2E Only > +# some image format support it, for example, qcow2, rbd, and sh= eepdog. s/format/formats/ > +# > +# Since: 1.6 > +## > +{ 'type': 'BlockdevSnapshotInternal', > + 'data': { 'device': 'str', 'name': 'str' } } > =20 > ## > # @TransactionAction > @@ -1623,6 +1638,7 @@ > { 'union': 'TransactionAction', > 'data': { > 'blockdev-snapshot-sync': 'BlockdevSnapshot' > + 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal' Invalid JSON - you need a comma between name-value pairs. (I _wish_ JSON had allowed trailing commas, the way C99 does, because it reduces the size of diffs when adding an element at the end...) Yet another instance of the introspection problem. Is there some other patch in this series that introduces a standalone command that witnesses whether this transaction is available? If so, I'm okay; if not, then libvirt won't know if this transaction is available without introspection, or by trying it first and detecting the error when it is not available. For the purposes of transactions, since any failure rolls back the entire transaction (including the failure of an unknown action within the transaction), the try-and-fail approach may be sufficient, even if it is not the prettiest. > +++ b/qmp-commands.hx > @@ -948,13 +948,13 @@ transaction > ----------- > =20 > Atomically operate on one or more block devices. The only supported > -operation for now is snapshotting. If there is any failure performing= > -any of the operations, all snapshots for the group are abandoned, and > -the original disks pre-snapshot attempt are used. > +operations for now are internal and external snapshotting. A list of > +dictionaries is accepted, that contains the actions to be performed. = The > +sequence of the requests will not affect the result. Not true. Taking an internal and then an external snapshot has a different effect than taking an external and then internal snapshot of the same disk. The sequence of requests is significant, because it controls which of two files involved contains the internal snapshot. > =20 > -A list of dictionaries is accepted, that contains the actions to be pe= rformed. > -For snapshots this is the device, the file to use for the new snapshot= , > -and the format. The default format, if not specified, is qcow2. > +For external snapshot, The dictionary is the device, the file to use f= or the s/snapshot, The/snapshots, the/ s/dictionary is/dictionary contains/ > +new snapshot, and the format. The default format, if not specified, i= s qcow2. > =20 > Each new snapshot defaults to being created by QEMU (wiping any > contents if the file already exists), but it is also possible to reuse= > @@ -963,6 +963,18 @@ the new image file has the same contents as the cu= rrent one; QEMU cannot > perform any meaningful check. Typically this is achieved by using the= > current image file as the backing file for the new image. > =20 > +On fail the original disks pre-snapshot attempt will be used. > + > +For internal snapshot, The dictionary is the device and the snapshot's= name. s/snapshot, The/snapsohts, the/ s/dictionary is/dictionary contains/ > +If name is a numeric string which will mess up with ID, the request wi= ll be > +rejected. For example, name "99" is not a valid name. If an internal= snapshot > +matching name already exists, the request will be also rejected. Only= some > +image format support it, for example, qcow2, rbd, and sheepdog. > + > +On fail, qemu will try delete new created internal snapshot in the s/fail/failure/ why "try"? The point of a transaction is that it either completely happens or is completely aborted. If you are leaving behind garbage after a failure later in the transaction, then you didn't design the transaction correctly. (And yes, we have an existing bug where external snapshots that get aborted after creating an empty destination file are leaving behind garbage, but that's pre-existing and unrelated to your feature addition.) > +transaction. When I/O error make delete fail, user need to fix it lat= er s/make delete fail/causes deletion failure/ s/user need/the user needs/ --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2QRCWSLHAIEQHMBPRXQOB 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.13 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJRvDZ9AAoJEKeha0olJ0NqyMkIAJboysdz9jBDXZjdD4bu7oeH 6GFdigPYVflKXSKCDDGqxjmJ8WuqZ5xruWS6CjrekgYtOQyPVBi1ZC57zcDALGVN NdCbXou7jXVVuIzKKQjCCKaRc6Qq/XKKobH34Pi0kgskKefd65kL0Jl74WB67BX3 mePhqhP+rD/MB7KLRbrL8jdbsifOwNmS45LNRIb5Kg8aEbrUfpBbbccc5v0QLtf8 KWd83SWJ1XPLsfgIs7GDGB1k7X1O90YCadWti7J6fR8fizZpevh6ZTY0i3kw7YTB 8dvgfcDBAIXrbihRQuqe99GBEXjOq43Y79uUVheRfFikwBq0wsCrSATQJFLuUaE= =xRut -----END PGP SIGNATURE----- ------enig2QRCWSLHAIEQHMBPRXQOB--