From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54687) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtfWm-0006QG-Pc for qemu-devel@nongnu.org; Wed, 26 Nov 2014 11:33:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XtfWh-0005ff-F0 for qemu-devel@nongnu.org; Wed, 26 Nov 2014 11:33:16 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46738) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtfWh-0005fN-68 for qemu-devel@nongnu.org; Wed, 26 Nov 2014 11:33:11 -0500 Message-ID: <547600BD.2080600@redhat.com> Date: Wed, 26 Nov 2014 09:33:01 -0700 From: Eric Blake MIME-Version: 1.0 References: <1416487488-8423-1-git-send-email-mreitz@redhat.com> <1416487488-8423-3-git-send-email-mreitz@redhat.com> In-Reply-To: <1416487488-8423-3-git-send-email-mreitz@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="cwwFLNE1pRRJQp31uoMF1XqQgE18FelEw" Subject: Re: [Qemu-devel] [PATCH 2/3] qmp: Expose read-only option for 'change' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: Kevin Wolf , Peter Lieven , Markus Armbruster , Stefan Hajnoczi , Luiz Capitulino This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --cwwFLNE1pRRJQp31uoMF1XqQgE18FelEw Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/20/2014 05:44 AM, Max Reitz wrote: > Expose the new read-only option of qmp_change_blockdev() for the > 'change' QMP command. Leave it unset for HMP for now. >=20 > Signed-off-by: Max Reitz > --- > hmp.c | 2 +- > qapi-schema.json | 7 ++++++- > qmp-commands.hx | 24 +++++++++++++++++++++++- > qmp.c | 15 ++++++++++++--- > 4 files changed, 42 insertions(+), 6 deletions(-) >=20 > diff --git a/hmp.c b/hmp.c > index 94b27df..0719fa3 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -1133,7 +1133,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)= > } > } > =20 > - qmp_change(device, target, !!arg, arg, &err); > + qmp_change(device, target, !!arg, arg, false, 0, &err); Passing raw '0' looks a bit odd for an enum value, but since the has_read_only parameter is false, I guess it doesn't matter. > if (strcmp(device, "vnc") =3D=3D 0) { > + if (has_read_only) { > + error_setg(errp, "Parameter 'read-only' is invalid for VNC= "); > + return; > + } The 'change' command is quite ugly. It would be nice if we could convert it to an automatic union, where the 'device' parameter is the discriminated union that determines whether we support or reject the optional 'read-only' argument - except that it is not a true discriminator (it is either 'vnc' or a device name, with the further implication that users CANNOT name their block device 'vnc' and still be able to use this QMP command). I wonder if it would be better to leave the ugly command alone, and instead introduce a new command that isn't multiplexed. If someone wants to set a disk's read-only status on change, then they MUST use the nice new command, and leave the old ugly command for backward compatibility only with no modifications to make it even uglier. Furthermore, adding a new command would let someone name their block device 'vnc' (not that libvirt has any plans to do so). At any rate, if we decide to live with extending the existing ugly 'change' command, your patch is correct, so here's a reluctant: Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --cwwFLNE1pRRJQp31uoMF1XqQgE18FelEw 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 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg iQEcBAEBCAAGBQJUdgC9AAoJEKeha0olJ0NqQNMH/jKk44quRqZPjyrvfh30EyZ3 L5BC2KYN4vIhykvh+qKt7Uml5RQngG5xM3Dmepd5Dfs4I6PYqFFfw5Po/gwtAsSE DzX/N9t63DwDSc2CdY64b+KVxM69lqFA9esFsbVI50VfxiCK7fdzU3HuBAaV/ygd wZMzsV9nW1TVqp4zHW7+JebqrTTfcLpRWWNavu3VB/feak+H9UEfuWfBLdaidNrX rF5/F9xJcReYJRgcUlBT2pmCK5fUVw/a8ngfdUIq2JQugES68XotGpgcCHiala73 Ndue2iyKlwzndaFl+qAl8mUkZ1xIVhCm066KrcyMPZgMBjgP0VjpODwbfz+/hFg= =VmVf -----END PGP SIGNATURE----- --cwwFLNE1pRRJQp31uoMF1XqQgE18FelEw--