From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35563) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZaSae-0001oC-Jp for qemu-devel@nongnu.org; Fri, 11 Sep 2015 13:58:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZaSad-0004oM-8G for qemu-devel@nongnu.org; Fri, 11 Sep 2015 13:58:24 -0400 References: <40f3f51fb44da70929f7055e95582351d1aa3f34.1441890725.git.berto@igalia.com> From: Eric Blake Message-ID: <55F31633.6010800@redhat.com> Date: Fri, 11 Sep 2015 11:58:11 -0600 MIME-Version: 1.0 In-Reply-To: <40f3f51fb44da70929f7055e95582351d1aa3f34.1441890725.git.berto@igalia.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="D9tet23jeVcXkvIOn7gJCxdbrKb12ngK0" Subject: Re: [Qemu-devel] [PATCH v3 3/4] block: add a 'blockdev-snapshot' QMP command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi , qemu-block@nongnu.org, Max Reitz This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --D9tet23jeVcXkvIOn7gJCxdbrKb12ngK0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/10/2015 07:39 AM, Alberto Garcia wrote: > One of the limitations of the 'blockdev-snapshot-sync' command is that > it does not allow passing BlockdevOptions to the newly created > snapshots, so they are always opened using the default values. >=20 > Extending the command to allow passing options is not a practical > solution because there is overlap between those options and some of > the existing parameters of the command. >=20 > This patch introduces a new 'blockdev-snapshot' command with a simpler > interface: it just takes two references to existing block devices that > will be used as the source and target for the snapshot. >=20 > Since the main difference between the two commands is that one of them > creates and opens the target image, while the other uses an already > opened one, the bulk of the implementation is shared. >=20 > Signed-off-by: Alberto Garcia > --- > blockdev.c | 163 ++++++++++++++++++++++++++++++++-----------= -------- > qapi-schema.json | 2 + > qapi/block-core.json | 26 ++++++++ > qmp-commands.hx | 29 +++++++++ > 4 files changed, 160 insertions(+), 60 deletions(-) >=20 > diff --git a/blockdev.c b/blockdev.c > index 6b787c1..78cfb79 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1183,6 +1183,18 @@ void qmp_blockdev_snapshot_sync(bool has_device,= const char *device, > &snapshot, errp); > } > =20 > +void qmp_blockdev_snapshot(const char *device, const char *snapshot, > + Error **errp) > +{ > + BlockdevSnapshot snapshot_data =3D { > + .device =3D (char *) device, > + .snapshot =3D (char *) snapshot > + }; Hmm. Sounds like you'd love to use my pending 'box':true qapi glue to make this function have the saner signature of void qmp_blockdev_snapshot(BlockdevSnapshot *arg, Error **errp); rather than having to rebuild the struct yourself (with an annoying cast to lose const) :) https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02599.html But no need to hold this series up waiting for the qapi review queue to flush. We can simplify later. > =20 > - options =3D qdict_new(); > - if (has_snapshot_node_name) { > - qdict_put(options, "node-name", > - qstring_from_str(snapshot_node_name)); > + if (snapshot_node_name && bdrv_find_node(snapshot_node_name)) = { > + error_setg(errp, "New snapshot node name already existing"= ); Pre-existing, but s/existing/exists/ while you are reindenting this. > +++ b/qapi/block-core.json > @@ -705,6 +705,19 @@ > '*format': 'str', '*mode': 'NewImageMode' } } > =20 > ## > +# @BlockdevSnapshot > +# > +# @device: device or node name to generate the snapshot from. > +# > +# @snapshot: reference to the existing block device that will be used > +# for the snapshot. Maybe mention that it must NOT have a current backing file, and point to the "backing":"" trick to get it that way. > +Create a snapshot, by installing 'device' as the backing image of > +'snapshot'. Additionally, if 'device' is associated with a block > +device, the block device changes to using 'snapshot' as its new active= > +image. Still didn't answer the question from the earlier review of whether the blockdev-snapshot-sync behavior of specifying the node name of an active layer in order to not pivot the block device to the snapshot still makes sense to support in the blockdev-snapshot case. But we could always add an optional boolean flag later if someone comes up for a use case where they'd need to create a snapshot of the active layer without the block device pivoting, so I don't think it should hold up this patch. > + > +Arguments: > + > +- "device": snapshot source (json-string) > +- "snapshot": snapshot target (json-string) > + > +Example: > + > +-> { "execute": "blockdev-snapshot", "arguments": { "device": "ide-hd0= ", > + "snapshot": "node1= 534" } } > +<- { "return": {} } Maybe enhance the example to show the preliminary blockdev-add that created node1534? I've pointed out some potential wording improvements, but think they are minor enough that you can have: Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --D9tet23jeVcXkvIOn7gJCxdbrKb12ngK0 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJV8xYzAAoJEKeha0olJ0NquNcIAI3fnYqVlWOnacDA1EeK5GjL sBguXnmH7VEXFMuea1uV9fWtvvOO/hT13tIJuoZWwLBV+lsulYeil5ZkiS3PR9ez JzJpvxkCHGqoys+V7viaufimpVQ97wnTjSqsISWYWHBq93G+JbB+XcDxXNvVDCh6 BK/qdX97S5FBeo2qfUCzKR15F3bOzN+mQGqKE41hTzAAlqMs0bbbQ7pfLts1/TaO 6SgM5yKxPsd0AS4AWNQNU2PiNznVvn87K485jim8fh4qezVc2+irBl73XNlQON0p YRH90Q/Mc7HA261O6FV1i5OdL+L1RimfZJKrC+aLhmpwosDPCsznO8xaqKTfvXQ= =Q5Dl -----END PGP SIGNATURE----- --D9tet23jeVcXkvIOn7gJCxdbrKb12ngK0--