From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50675) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVMtE-0004zN-AL for qemu-devel@nongnu.org; Thu, 25 Apr 2013 10:11:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UVMt6-00075u-Vw for qemu-devel@nongnu.org; Thu, 25 Apr 2013 10:11:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54002) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVMt6-00075O-OD for qemu-devel@nongnu.org; Thu, 25 Apr 2013 10:11:04 -0400 Message-ID: <51793977.4080602@redhat.com> Date: Thu, 25 Apr 2013 08:11:03 -0600 From: Eric Blake MIME-Version: 1.0 References: <1366883435-4993-1-git-send-email-akong@redhat.com> <20130425092050.41428940@redhat.com> In-Reply-To: <20130425092050.41428940@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2NRMENOFNLXEQFUTGSXRE" Subject: Re: [Qemu-devel] [PATCH v5] monitor: introduce query-command-line-options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: aliguori@us.ibm.com, Amos Kong , qemu-devel@nongnu.org, jyang@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2NRMENOFNLXEQFUTGSXRE Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 04/25/2013 07:20 AM, Luiz Capitulino wrote: > On Thu, 25 Apr 2013 17:50:35 +0800 > Amos Kong wrote: >=20 >> Libvirt has no way to probe if an option or property is supported, >> This patch introduces a new qmp command to query command line >> option information. hmp command isn't added because it's not needed. >> >> Signed-off-by: Amos Kong >> CC: Luiz Capitulino >> CC: Osier Yang >> CC: Anthony Liguori >=20 > This version looks good to me, but it has two small problems. >=20 > First, some options like -drive, -net, -netdev and -device have an empt= y > QemuOptsList, meaning that option validation is not done upfront. >=20 > The other problem is that we're returning some options which seem to be= > created at run-time and I honestly have never heard of them, like tmpde= v > and acpi. >=20 > I think there are two possible fixes for both issues: >=20 > 1. Do nothing, accept patch as is and fix things in 1.6 I can live with this for 1.5. Initially, libvirt will probably only be querying whether a set of known option names exist, and not looking at the parameter definitions tied to the option nor trying to invoke command line options that libvirt doesn't already know. Put another way, the driving factor to get this into 1.5 is so that libvirt knows whether it can use -mem-merge: https://www.redhat.com/archives/libvir-list/2013-April/msg01263.html While I anticipate that future libvirt versions may get fancier and use even more details of option introspection, such as learning whether a parameter was added in a later release, that's not the driving factor right now. >=20 > 2. Don't return an option when 'desc' is NULL That might backfire; it's better to output too much than too little. >=20 > I'm willing to do 1, as those options don't return any options anyway a= nd > I don't believe any client will mess with them (worst case the client w= ill pass > an option qemu doesn't accept and will refuse to run). Yep, and libvirt certainly won't be calling 'qemu -tmpdev'. >=20 > I have one more comment below, but case we accept to do 1 I can fix it = myself. >=20 >> +{'command': 'query-command-line-options', 'data': { '*option': 'str' = }, >=20 > I'm not a huge fan of option being optional, I prefer dropping it and m= aking > the command simpler. But I won't refuse the patch because of that. I'm leaving that up to you. I personally like the idea of filtering (I would definitely use it via 'virsh qemu-monitor-command' when doing development work on libvirt), but it's not essential. >> + if (conf_list =3D=3D NULL) { >> + error_set(errp, QERR_INVALID_OPTION_GROUP, option); >=20 > You should be using error_setg() like: >=20 > error_setg(errp, "invalid option name: %s", option); >=20 > But I can fix that myself in qmp branch. Based on include/qapi/qmp/qerror.h, the message you would be replacing is= : #define QERR_INVALID_OPTION_GROUP \ ERROR_CLASS_GENERIC_ERROR, "There is no option group '%s'" Yeah, "invalid option name: foo" reads better than "There is no option group 'foo'". --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2NRMENOFNLXEQFUTGSXRE 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/ iQEcBAEBCAAGBQJReTl3AAoJEKeha0olJ0NqdfQH/il8tCGPMUsbMrYrF0IC8knR JuWbfaUpF7u+AxHJl6+73xxRqZkFQrbH+pUOcewWygFV15UmwnEWbY3H+b5hbT/M lwgkTMeL9CaHs3z29KCI7ctHFGufMo0DOvMZTElWLr0PGCq1YkBrLQ7MYJJarG/a VETvU4UoUHacsUSJ49D9OY7yBUzWnybGIC91DVAL5+NdJDuDCS7dArgUdRUw4kLw 4Z0EarfmSmkv+LBMGxRxh8Hw4+qAgDS+eTQhUABq3Yqc2M2kfrEZZGhn4wMfFynL /EHvYkAvIKqMS1knFDA50OO9MPFLgnqggMrhQfXCkV6kvN+viq8Eg1dFmctNtRw= =EgI4 -----END PGP SIGNATURE----- ------enig2NRMENOFNLXEQFUTGSXRE--