From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55788) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGa4X-0005Bg-2s for qemu-devel@nongnu.org; Wed, 28 Jan 2015 16:22:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YGa4V-0004Xh-Uj for qemu-devel@nongnu.org; Wed, 28 Jan 2015 16:22:49 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51928) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGa4V-0004XX-Ni for qemu-devel@nongnu.org; Wed, 28 Jan 2015 16:22:47 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t0SLMlnL008949 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 28 Jan 2015 16:22:47 -0500 Message-ID: <54C95325.8080501@redhat.com> Date: Wed, 28 Jan 2015 14:22:45 -0700 From: Eric Blake MIME-Version: 1.0 References: <1422387983-32153-1-git-send-email-mreitz@redhat.com> <1422387983-32153-49-git-send-email-mreitz@redhat.com> In-Reply-To: <1422387983-32153-49-git-send-email-mreitz@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Ng3chNTnNuIvG7aDWaJqxRem51CoQ3WRB" Subject: Re: [Qemu-devel] [PATCH RESEND 48/50] hmp: Add read-only option to change command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: Kevin Wolf , Fam Zheng , Jeff Cody , Markus Armbruster , Stefan Hajnoczi , John Snow This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --Ng3chNTnNuIvG7aDWaJqxRem51CoQ3WRB Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 01/27/2015 12:46 PM, Max Reitz wrote: > Expose the new read-only option of 'blockdev-change-medium' for the > 'change' HMP command. >=20 > Signed-off-by: Max Reitz > Reviewed-by: Eric Blake > --- > hmp-commands.hx | 20 +++++++++++++++++--- > hmp.c | 21 ++++++++++++++++++++- > 2 files changed, 37 insertions(+), 4 deletions(-) >=20 Already reviewed... > =20 > +@var{read-only} may be used to change the read-only status of the devi= ce. It > +accepts the following values: > + > +@table @var > +@item retain > +Retains the current status; this is the default. > + > +@item ro > +Makes the device read-only. > + > +@item rw > +Makes the device writable. Hmm. I suggested in 47/50 to rename the QMP enum values to something longer, which would affect this on a rebase. On the other hand, it would be nice to make the HMP counterpart support BOTH spellings for convenience ('read-write' for legibility, 'rw' for brevity in typing); and I have no clue if tab completion starts to play a role either. Up to you if you want to add complexity or leave things simple and stupid (the choices we make in HMP for user-friendliness have no bearing on what libvirt will want to do, so I really have no strong preference). =2E..so my review still stands if nothing changes, and is probably worth dropping if you respin to add user-friendly complexity. > const char *arg =3D qdict_get_try_str(qdict, "arg"); > + const char *read_only =3D qdict_get_try_str(qdict, "read-only"); > + BlockdevChangeReadOnlyMode read_only_mode =3D 0; > Error *err =3D NULL; > =20 > if (strcmp(device, "vnc") =3D=3D 0) { > + if (read_only) { > + monitor_printf(mon, "Parameter 'read-only' is invalid for = VNC"); Hmm. In HMP, you don't type the parameter name (it is implicit based on the rules for converting positional HMP arguments into dictionary entries for use in the rest of the code). In particular, that means that if I type 'change vnc password rw', that means that 'rw' will be in the "read-only" parameter position, even though it has nothing to do with read-only. [hmm - how did 'change vnc password XXX' work anyways, since the .params for 'change' didn't allow a third argument before your patch?] At any rate, the idea I mentioned in the QMP patch about naming the parameter 'read-mode' instead of 'read-only' might make for nicer error messages. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --Ng3chNTnNuIvG7aDWaJqxRem51CoQ3WRB 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 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJUyVMlAAoJEKeha0olJ0NqsroIAJixq3SOtawpClwQ9nkPoV3I u6gVorFeBxQnwDZ5pP/mnvJL3PsRSq63zIH69D2ZMzf+4wQXeAgx6PkYaxr3ZQav 20UzYVcNvwp5S/DBB3ng3lWWU277JnMRlftufxhdUTzmiDFnQXSh6yaOSxrUC4dL Miifpyv6KGqCaSNvTDrdDo4tXhw1AcLfNoQvSedTOZf6MIzufNLkAKwWwdajk0+4 Kktq9JgEgyZJE2j5+lvmPA2WhX1YqXA62CtpgW5Wjm+IsxaxGZv4CjOwfIYcbGfn GBECiWxtXVJucVj4jIdmiAq7MGBDVKqTvOIBF85ZkLJpWviKaIAXNw1vEMUDrAs= =qEC3 -----END PGP SIGNATURE----- --Ng3chNTnNuIvG7aDWaJqxRem51CoQ3WRB--