From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37861) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Unn9O-0005zF-IQ for qemu-devel@nongnu.org; Sat, 15 Jun 2013 05:52:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Unn9N-0006o1-8o for qemu-devel@nongnu.org; Sat, 15 Jun 2013 05:52:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:21476) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Unn9N-0006nx-1H for qemu-devel@nongnu.org; Sat, 15 Jun 2013 05:52:01 -0400 Message-ID: <51BC393A.8030106@redhat.com> Date: Sat, 15 Jun 2013 10:51:54 +0100 From: Eric Blake MIME-Version: 1.0 References: <1371209999-15579-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1371209999-15579-9-git-send-email-xiawenc@linux.vnet.ibm.com> In-Reply-To: <1371209999-15579-9-git-send-email-xiawenc@linux.vnet.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2NIOJGIJNCLAFEQPJQDXE" Subject: Re: [Qemu-devel] [PATCH V2 08/12] qmp: add interface blockdev-snapshot-internal-sync 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) ------enig2NIOJGIJNCLAFEQPJQDXE Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 06/14/2013 12:39 PM, Wenchao Xia wrote: > This interface can generate snapshot name automatically if it is not > specified, since it is a single opertion. s/opertion/operation/ >=20 > Snapshot ID can't be specified in this interface. >=20 > Signed-off-by: Wenchao Xia > --- > blockdev.c | 25 +++++++++++++++++++++++++ > qapi-schema.json | 23 +++++++++++++++++++++++ > qmp-commands.hx | 31 +++++++++++++++++++++++++++++++ > 3 files changed, 79 insertions(+), 0 deletions(-) >=20 > +++ b/qapi-schema.json > @@ -1689,6 +1689,29 @@ > '*mode': 'NewImageMode'} } > =20 > ## > +# @blockdev-snapshot-internal-sync > +# > +# Synchronously take an internal snapshot of a block device, when the = format > +# of the image used supports it. > +# > +# @device: the name of the device to generate the snapshot from > +# > +# @name: #optional the new snapshot name. If not specified, a name wil= l be > +# generated according to time by qemu So why is name optional here but mandatory within a transaction? If qemu is able to generate names, then it should be able to generate names in both cases. Otherwise, make the name mandatory in both places. Should this patch be folded in to 7/12? Compare with Stefan's series on adding block-snapshot as a transaction (for that matter, the two series have a [trivial] merge conflict since both add a transaction), and make sure you are using the same approach between the two series at introducing things. > +# > +# Returns: nothing on success > +# If @device is not a valid block device, DeviceNotFound > +# If any snapshot matching @name exists, or the name is a num= eric > +# which may mess up with snapshot ID, generic error will be r= eturned > +# If the format of the image used does not support it, > +# BlockFormatFeatureNotSupported > +# > +# Since 1.6 > +## > +{ 'command': 'blockdev-snapshot-internal-sync', > + 'data': { 'device': 'str', '*name': 'str'} } Ah, so this answers my question in 7/12 about a witness that libvirt can use for knowing when transaction supports the new action, without needing introspection. > + > +SQMP > +blockdev-snapshot-internal-sync > +------------------------------- > + > +Synchronously take an internal snapshot of a block device when the for= mat of > +image used supports it. If name is not specified, it will be automati= cally > +generated by qemu according to host time. If the name is a numeric st= ring > +which may mess up with ID, such as "19", the operation will fail. Wait a second. If we DON'T pass a name, then the generated name is all numeric. But if we DO pass an all-numeric name, it gets rejected. That's awkward to explain. Maybe you want to instead have a cutoff, where a number < 64k (is that the right threshold? I just picked a number out of the air) is rejected, but a number >=3D 64k is treated as valid because it might represent a timestamp. Or allow all possible numbers, and only reject the creation of a name that collides with an existing id. It may be too hard to predict if a name will collide with a future id of a later snapshot operation. > +Example: > + > +-> { "execute": "blockdev-snapshot-internal-sync", > + "arguments": { "device": "ide-hd0", > + "name": "snapshot0" } > + } > +<- { "return": {} } Evil. If I don't pass a name, then I NEED to know what name got generated on my behalf. So that argues you need to return something, rather than nothing. I can see why you can't return a string via 'transaction', but maybe this is an argument that 'name' should be mandatory in this QMP command (and any generation of a timestamp id must be higher up in the stack, at the HMP level, so that HMP can still treat name as optional). But then you are back to solving the problem of allowing an all-numeric generated timestamp as a valid name. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2NIOJGIJNCLAFEQPJQDXE 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/ iQEcBAEBCAAGBQJRvDk6AAoJEKeha0olJ0NqIyAH/1CnycjUmV724sU4fVm/uHTt CI5mKBc58+q24f1a2lgcarTSMurJsM+0tEcD5T0b+zKKsezOCVRG6gc+1hu93jAS 8trX7b4NuSdeNT4JAMDWYYPDNzvg0vfSwB2wTtSQ1Hz4pzp8QHHzDzjKGRk4OzKL T+ej6UWdzgDRLNWBT1xyTqEpCtaycYI1S9IclPVLZg+nwMETjezvG7XfEbWUTiDB utEn9cTIK1fu+DTZLyfPmXb2RtpbfCyEXIuDzGWY4xPc9uAgF19nggDDAQk4Z0aN TX0mxamsKFcxs3LZG2idTOQPZWafm2hEoZCuR7DtGDX65QjRzo8KSKTYXv8j944= =iv7D -----END PGP SIGNATURE----- ------enig2NIOJGIJNCLAFEQPJQDXE--