From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47841) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKxQl-0007qS-1y for qemu-devel@nongnu.org; Tue, 04 Mar 2014 17:03:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WKxQg-0000GO-60 for qemu-devel@nongnu.org; Tue, 04 Mar 2014 17:03:19 -0500 Received: from mx1.redhat.com ([209.132.183.28]:11156) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKxQf-0000G0-Rb for qemu-devel@nongnu.org; Tue, 04 Mar 2014 17:03:14 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s24M3Civ007056 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 4 Mar 2014 17:03:12 -0500 Message-ID: <53164D9C.4080500@redhat.com> Date: Tue, 04 Mar 2014 15:03:08 -0700 From: Eric Blake MIME-Version: 1.0 References: <1393912317-26221-1-git-send-email-akong@redhat.com> <1393912317-26221-3-git-send-email-akong@redhat.com> In-Reply-To: <1393912317-26221-3-git-send-email-akong@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="UrapBeRq8HWv23WG4PsK0BVHwVmQgW2rl" Subject: Re: [Qemu-devel] [PATCH v3 2/2] query-command-line-options: query all the options in qemu-options.hx List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amos Kong , qemu-devel@nongnu.org Cc: libvir-list@redhat.com, pbonzini@redhat.com, armbru@redhat.com, jyang@redhat.com, lcapitulino@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --UrapBeRq8HWv23WG4PsK0BVHwVmQgW2rl Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 03/03/2014 10:51 PM, Amos Kong wrote: > vm_config_groups[] only contains part of the options which have > argument, and all options which have no argument aren't added to > vm_config_groups[]. Current query-command-line-options only > checks options from vm_config_groups[], so some options will > be lost. >=20 > We have some macros in qemu-options.hx to generate a table that > contains all the options. This patch tries to query options from > the table. >=20 > Then we won't lose the legacy options that weren't added to > vm_config_groups[] (eg: -vnc, -smbios). The options that have > no argument will also be returned (eg: -enable-fips) >=20 > Some options that have argument have a NULL desc list, some > options don't have argument, and "parameters" is mandatory > in the past. So we add a new field "arguments" to present Here you call it "arguments", but in the code you call it "argument". > if the option takes unspecified arguments. >=20 > Signed-off-by: Amos Kong > --- > qapi-schema.json | 8 ++++++-- > qemu-options.h | 18 ++++++++++++++++++ > util/qemu-config.c | 35 +++++++++++++++++++++++++++++------ > vl.c | 17 ----------------- > 4 files changed, 53 insertions(+), 25 deletions(-) Umm, did you test this? $ printf %s\\n \ '{"execute":"qmp_capabilities"}' \ '{"execute":"query-command-line-options"}' \ '{"execute":"quit"}' \ | ./x86_64-softmmu/qemu-system-x86_64 -qmp stdio | grep fips $ I was expecting -enable-fips to appear somewhere in the output. Something's not right, but I'm not going to figure out what. Here's hoping v4 actually gets it working. >=20 > diff --git a/qapi-schema.json b/qapi-schema.json > index 05ced9d..0bd8e12 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -3944,12 +3944,16 @@ > # > # @option: option name > # > -# @parameters: an array of @CommandLineParameterInfo > +# @parameters: array of @CommandLineParameterInfo, possibly empty > +# @argument: @optional present if the @parameters array is empty. If > +# true, then the option takes unspecified arguments, if > +# false, then the option is merely a boolean flag (since 2.= 0) For that matter, even this wasn't true. In my testing, I see the same thing as pre-patch for the -smbios option: {"parameters": [], "option": "smbios"} but the docs imply that I should now see: {"parameters": [], "option": "smbios", "argument": true} > +++ b/qemu-options.h > @@ -25,6 +25,8 @@ > * THE SOFTWARE. > */ > =20 > +#include "sysemu/arch_init.h" > + > #ifndef _QEMU_OPTIONS_H_ > #define _QEMU_OPTIONS_H_ > =20 > @@ -33,4 +35,20 @@ enum { > #include "qemu-options-wrapper.h" > }; > =20 > +#define HAS_ARG 0x0001 Defining this non-namespace-friendly macro in a header seems risky. Can you localize its use, by using it only in the .c file that needs it, and/or #undef it when done using it? > + > +typedef struct QEMUOption { > + const char *name; > + int flags; > + int index; > + uint32_t arch_mask; > +} QEMUOption; > + > +static const QEMUOption qemu_options[] =3D { > + { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, > +#define QEMU_OPTIONS_GENERATE_OPTIONS > +#include "qemu-options-wrapper.h" > + { NULL }, > +}; Sticking a static array in a header is even worse than the v2 - now every .c file that includes this .h has its own copy of the variable. You really want the .h to just declare the variable as extern, then have a single .c file actually implement it. > + for (i =3D 0; qemu_options[i].name; i++) { > + if (!has_option || !strcmp(option, qemu_options[i].name)) { > info =3D g_malloc0(sizeof(*info)); defaults info->has_argument to false and info->argument to false... > - info->option =3D g_strdup(vm_config_groups[i]->name); > - if (!strcmp("drive", vm_config_groups[i]->name)) { > + info->option =3D g_strdup(qemu_options[i].name); > + > + int idx =3D get_group_index(qemu_options[i].name); > + > + if (qemu_options[i].flags) { > + info->argument =3D true; > + } =2E..changes info->argument to true for options that take unspecified arguments (such as -smbios), but with no effect to output unless... > + > + if (!strcmp("drive", qemu_options[i].name)) { > info->parameters =3D get_drive_infolist(); > - } else { > + } else if (idx >=3D 0) { > info->parameters =3D > - get_param_infolist(vm_config_groups[i]->desc); > + get_param_infolist(vm_config_groups[idx]->desc); > + } > + > + if (!info->parameters) { > + info->has_argument =3D true; > } =2E..this line gets executed. I guess info->parameters is false for both= boolean options (where info->argument remains at its default of false) and for unspecified arguments (where info->argument was set to true above), while omitting the argument output for options that take named options? But while it looks okay in theory, the implementation was still broken based on my testing, so I'm not sure what went wrong. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --UrapBeRq8HWv23WG4PsK0BVHwVmQgW2rl 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/ iQEcBAEBCAAGBQJTFk2cAAoJEKeha0olJ0NqHEQIAJ97OIV6+cL8RKsFnMiDEBNL 6S9cQAHiC9bGwKcLX5R4mRiQmdRdEy51lsL60epib2IwjxItH7EnPq6m062thLKl icTettpUJczP2IvHOaoJVdC5aivO4tr9mBJTda9dAxsfwPXyn7u7cMcKd1czQawF HTtP2n4D2DkY7v35Dyerexnq0f52w+bnggvJQVWfGNQzIvDhcp8iGOG6jPWGT64E eiJndYqaEseNpimxIooPEzxvVciGnAtmJwoI+aiSaYXOouSE08qYlzo9g3hlfESR G4eGVBvwmXCMq7une5R5vhywLldhfrKYBKpvXf4YJ8k09fefCZ7WEm2TacO1QHg= =gH5X -----END PGP SIGNATURE----- --UrapBeRq8HWv23WG4PsK0BVHwVmQgW2rl--