From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51737) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWV9T-0007KA-HR for qemu-devel@nongnu.org; Mon, 31 Aug 2015 15:54:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZWV9S-0004zv-2n for qemu-devel@nongnu.org; Mon, 31 Aug 2015 15:53:59 -0400 References: <457103c2204e849aea3b83ffd78fad049d8d923d.1441014844.git.berto@igalia.com> From: Max Reitz Message-ID: <55E4B0CC.6070906@redhat.com> Date: Mon, 31 Aug 2015 21:53:48 +0200 MIME-Version: 1.0 In-Reply-To: <457103c2204e849aea3b83ffd78fad049d8d923d.1441014844.git.berto@igalia.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="MNBtoGlIDe9Q9no0LlADCSP6TkVscDNS9" Subject: Re: [Qemu-devel] [PATCH 1/1] block: Allow passing BlockdevOptions to blockdev-snapshot-sync 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 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --MNBtoGlIDe9Q9no0LlADCSP6TkVscDNS9 Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 31.08.2015 12:00, Alberto Garcia wrote: > Snapshots created using blockdev-snapshot-sync are currently opened > using their default options, not even inheriting those from the images > they are based on. >=20 > This patch extends the command by adding an 'options' parameter that > takes a BlockdevOptions structure. Since some of the options that can > be specified overlap with the parameters of blockdev-snapshot-sync, > the following checks are made: >=20 > - The 'driver' field must match the format specified for the snapshot. > - The 'node-name' field and the 'snapshot-node-name' parameters cannot > be specified at the same time. > - The 'file' field can only contain an empty string, since it overlaps > with the 'snapshot-file' parameter. >=20 > Signed-off-by: Alberto Garcia > --- > blockdev.c | 59 ++++++++++++++++++++++++++++++++++++++++++++= ++------ > hmp.c | 2 +- > qapi/block-core.json | 9 +++++++- > qmp-commands.hx | 3 ++- > 4 files changed, 64 insertions(+), 9 deletions(-) Design question: Would it make sense to instead add a "reference" mode to blockdev-snapshot-sync where you can specify a BDS's node-name instead of snapshot-file to use an existing BDS as the new top layer, ideally an empty one? What we'd then need is a QMP command for creating images. But as far as I know we'll need that anyway sooner or later... Comments on the patch itself below. > diff --git a/blockdev.c b/blockdev.c > index 4a1fc2e..7e904f3 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1070,7 +1070,9 @@ void qmp_blockdev_snapshot_sync(bool has_device, = const char *device, > bool has_snapshot_node_name, > const char *snapshot_node_name, > bool has_format, const char *format, > - bool has_mode, NewImageMode mode, Erro= r **errp) > + bool has_mode, NewImageMode mode, > + bool has_options, BlockdevOptions *opt= ions, > + Error **errp) > { > BlockdevSnapshot snapshot =3D { > .has_device =3D has_device, > @@ -1084,6 +1086,8 @@ void qmp_blockdev_snapshot_sync(bool has_device, = const char *device, > .format =3D (char *) format, > .has_mode =3D has_mode, > .mode =3D mode, > + .has_options =3D has_options, > + .options =3D options, > }; > blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC,= > &snapshot, errp); > @@ -1504,6 +1508,48 @@ static void external_snapshot_prepare(BlkTransac= tionState *common, > =20 > flags =3D state->old_bs->open_flags; > =20 > + if (action->blockdev_snapshot_sync->has_options) { > + QObject *obj; > + QmpOutputVisitor *ov =3D qmp_output_visitor_new(); > + visit_type_BlockdevOptions(qmp_output_get_visitor(ov), > + &action->blockdev_snapshot_sync->op= tions, > + NULL, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + qmp_output_visitor_cleanup(ov); > + return; > + } > + > + obj =3D qmp_output_get_qobject(ov); > + options =3D qobject_to_qdict(obj); > + qmp_output_visitor_cleanup(ov); > + > + if (strcmp(format, qdict_get_str(options, "driver"))) { With has_format =3D=3D false, format will be set to "qcow2" by default. S= o, if the user does not specify the format explicitly, the "driver" field has to be set to "qcow2". I'd rather make specifying @format and @options exclusive, and if @options has been specified, its "driver" field should override the "format" default. > + error_setg(errp, "Can't specify two different image format= s"); > + goto fail; > + } > + > + if (has_snapshot_node_name && qdict_haskey(options, "node-name= ")) { > + error_setg(errp, "Can't specify a node name twice"); > + goto fail; > + } > + > + obj =3D qdict_get(options, "file"); > + if (obj !=3D NULL) { > + QString *str =3D qobject_to_qstring(obj); > + if (!str || strcmp(qstring_get_str(str), "")) { > + error_setg(errp, "The 'file' field must be empty"); > + goto fail; > + } > + qdict_del(options, "file"); > + } And the "backing" field must be NULL. > + > + qdict_flatten(options); > + } else { > + options =3D qdict_new(); > + qdict_put(options, "driver", qstring_from_str(format)); > + } > + > /* create new image w/backing file */ > if (mode !=3D NEW_IMAGE_MODE_EXISTING) { > bdrv_img_create(new_image_file, format, > @@ -1512,19 +1558,15 @@ static void external_snapshot_prepare(BlkTransa= ctionState *common, > NULL, -1, flags, &local_err, false); > if (local_err) { > error_propagate(errp, local_err); > - return; > + goto fail; > } > } > =20 > - options =3D qdict_new(); > if (has_snapshot_node_name) { > qdict_put(options, "node-name", > qstring_from_str(snapshot_node_name)); > } > - qdict_put(options, "driver", qstring_from_str(format)); > =20 > - /* TODO Inherit bs->options or only take explicit options with an > - * extended QMP command? */ > assert(state->new_bs =3D=3D NULL); > ret =3D bdrv_open(&state->new_bs, new_image_file, NULL, options, > flags | BDRV_O_NO_BACKING, &local_err); > @@ -1532,6 +1574,11 @@ static void external_snapshot_prepare(BlkTransac= tionState *common, > if (ret !=3D 0) { > error_propagate(errp, local_err); > } > + > + return; > + > +fail: > + QDECREF(options); > } > =20 > static void external_snapshot_commit(BlkTransactionState *common) > diff --git a/hmp.c b/hmp.c > index dcc66f1..180a7f6 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -1115,7 +1115,7 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDic= t *qdict) > qmp_blockdev_snapshot_sync(true, device, false, NULL, > filename, false, NULL, > !!format, format, > - true, mode, &err); > + true, mode, false, NULL, &err); > hmp_handle_error(mon, &err); > } > =20 > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 7b2efb8..5eb111e 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -697,11 +697,18 @@ > # > # @mode: #optional whether and how QEMU should create a new image, def= ault is > # 'absolute-paths'. > +# > +# @options: #optional options for the new device, with the following > +# restrictions for the fields: 'driver' must match the value= > +# of @format, As said above, I'd rather make specifying both @options and @format exclusive. Maybe there is even some QAPI magic to enforce that (and for 'node-name', too), I don't know... Max > 'node-name' and @snapshot-node-name cannot be > +# specified at the same time, and 'file' can only contain an= > +# empty string (Since 2.5) > ## > { 'struct': 'BlockdevSnapshot', > 'data': { '*device': 'str', '*node-name': 'str', > 'snapshot-file': 'str', '*snapshot-node-name': 'str', > - '*format': 'str', '*mode': 'NewImageMode' } } > + '*format': 'str', '*mode': 'NewImageMode', > + '*options': 'BlockdevOptions' } } > =20 > ## > # @DriveBackup > diff --git a/qmp-commands.hx b/qmp-commands.hx > index ba630b1..bf45e31 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -1394,7 +1394,7 @@ EQMP > =20 > { > .name =3D "blockdev-snapshot-sync", > - .args_type =3D "device:s?,node-name:s?,snapshot-file:s,snapsh= ot-node-name:s?,format:s?,mode:s?", > + .args_type =3D "device:s?,node-name:s?,snapshot-file:s,snapsh= ot-node-name:s?,format:s?,mode:s?,options:q?", > .mhandler.cmd_new =3D qmp_marshal_input_blockdev_snapshot_sync= , > }, > =20 > @@ -1417,6 +1417,7 @@ Arguments: > - "mode": whether and how QEMU should create the snapshot file > (NewImageMode, optional, default "absolute-paths") > - "format": format of new image (json-string, optional) > +- "options": options for the new image (json-dict) > =20 > Example: > =20 >=20 --MNBtoGlIDe9Q9no0LlADCSP6TkVscDNS9 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 iQEcBAEBCAAGBQJV5LDMAAoJEDuxQgLoOKytuo8IAK0rHHbgqcGr11V/pCXlMckE lnvQmOaVnjivozcohLV40ujrVeG4rI5jc8bhFC0lQ88rRD6wtjtZK00bCQLR9WWq wOU/LY80LK6XK1P4mbt7nPxwThDiKwkMTOWoTQMWkS+n3tTDT1o3nP1XR/yP7tOt mlkve3SX4LoUDeOahYaL2u9THfN4F5aP2Zp31lS+lFRrayEGOAQILmsa3EHnZLJd EoTHSsIA4y/wJIIEXq3z8Z7fpnQc3v/bZryF4zAWS1OK3xiwPaTXjEBhemtK6N4D 72G+l1xng6QYSpw58O0SH/oQQPLejpCONhzgWleylESz6l9O1rfeM56zZBdzw6c= =W3pS -----END PGP SIGNATURE----- --MNBtoGlIDe9Q9no0LlADCSP6TkVscDNS9--