From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38590) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UV0q9-0007f8-Co for qemu-devel@nongnu.org; Wed, 24 Apr 2013 10:38:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UV0q3-0007W5-OS for qemu-devel@nongnu.org; Wed, 24 Apr 2013 10:38:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57820) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UV0q3-0007Vw-Gz for qemu-devel@nongnu.org; Wed, 24 Apr 2013 10:38:27 -0400 Message-ID: <5177EE61.7030909@redhat.com> Date: Wed, 24 Apr 2013 08:38:25 -0600 From: Eric Blake MIME-Version: 1.0 References: <1366807646-8473-1-git-send-email-akong@redhat.com> <1366807646-8473-2-git-send-email-akong@redhat.com> In-Reply-To: <1366807646-8473-2-git-send-email-akong@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2OFRQJVHQXMLSNTWLLBKA" Subject: Re: [Qemu-devel] [PATCH 2/2] monitor: introduce query-config-schema command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amos Kong Cc: pbonzini@redhat.com, aliguori@us.ibm.com, qemu-devel@nongnu.org, jyang@redhat.com, lcapitulino@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2OFRQJVHQXMLSNTWLLBKA Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 04/24/2013 06:47 AM, Amos Kong wrote: > Libvirt has no way to probe if an option or property is supported, > This patch introdues a new qmp command to query configuration schema > information. hmp command isn't added because it's not needed. Agreed that no HMP counterpart is needed. However, I don't think we have quite the right interface yet. >=20 > Signed-off-by: Amos Kong > CC: Osier Yang > CC: Anthony Liguori > --- > qapi-schema.json | 29 +++++++++++++++++++++++++++++ > qmp-commands.hx | 40 ++++++++++++++++++++++++++++++++++++++++ > util/qemu-config.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 109 insertions(+), 0 deletions(-) >=20 > diff --git a/qapi-schema.json b/qapi-schema.json > index 751d3c2..aeab057 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -3505,3 +3505,32 @@ > '*asl_compiler_rev': 'uint32', > '*file': 'str', > '*data': 'str' }} > + > +## > +# @ConfigSchemaInfo: > +# > +# Configration schema information. > +# > +# @option: option name > +# > +# @params: parameters strList of one option Why just a strList? That only tells me option names. But we already know so much more than that - we know the type and the help string. > +# > +# Since 1.5 > +## > +{ 'type': 'ConfigSchemaInfo', 'data': {'option': 'str', 'params': ['st= r']} } I'd rather see an array of structs, more closely mirroring what include/qemu/option.h gives us: # JSON representation of values of QEMUOptionParType, may grow in future { 'enum': 'ConfigParamType', 'data': [ 'flag', 'number', 'size', 'string' ] } # JSON representation of QEMUOptionParameter, may grow in future # @help is optional if no text was present { 'type': 'ConfigParamInfo', 'data': { 'name': 'str', 'type': 'ConfigParamType', '*help':'str' } } # Each command line option, and its list of parameters { 'type': 'ConfigSchemaInfo', 'data': { 'option':'str', 'params': ['ConfigParamInfo'] } } And that means we no longer have ['str'], which bypasses the need for your patch 1/2. > + > +## > +# @query-config-schema > +# > +# Query configuration schema information of options > +# > +# @option: #optional option name > +# > +# Returns: returns @ConfigSchemaInfo if option is assigned, returns > +# @ConfigSchemaInfo list if no option is assigned, returns an= error > +# QERR_INVALID_OPTION_GROUP if assigned option doesn't exist.= That didn't read well. Also, QERR_INVALID_OPTION_GROUP is a generic error; we don't mention any other QERR_* names in qapi-schema.json. How about: Returns: list of @ConfigSchemaInfo for all options (or for the given @option). Returns an error if a given @option doesn't exist. > +# > +# Since 1.5 > +## > +{'command': 'query-config-schema', 'data': {'*option': 'str'}, > + 'returns': ['ConfigSchemaInfo']} This part looks good. > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 4d65422..c6399be 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -2414,6 +2414,46 @@ EQMP > .args_type =3D "", > .mhandler.cmd_new =3D qmp_marshal_input_query_uuid, > }, > +SQMP > +query-config-schema > +------------ > + > +Show configuration schema. > + > +Return configuration schema of one option if option is assigned, retur= n > +configuration schema list of all options if no option is assigned. ret= urn > +an error QERR_INVALID_OPTION_GROUP if assigned option doesn't exist. Again, QERR_INVALID_OPTION_GROUP is not a defined error (it is shorthand for a specific message for a GenericError class). > + > +- "option": option name > +- "params": parameters string list of one option > + > +Example: > + > +-> {"execute": "query-config-schema", "arguments" : {"option": "boot-o= pts"}} > +<- { > + "return": [ > + { > + "params": [ > + "strict", > + "reboot-timeout", > + "splash-time", > + "splash", > + "menu", > + "once", > + "order" > + ], > + "option": "boot-opts" > + } > + ] > + } Back to my desire for more structure, I'd like to see: -> {"execute": "query-config-schema", "arguments" : {"option": "boot-opts= "}} <- { "return": [ { "params": [ { "name": "strict", "type": "string" }, { "name": "reboot-timeout", "type": "string" }, ... ], "option": "boot-opts" } ] } Another more interesting example, showing why the "type" and optional "help" fields are useful, assuming https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg04810.html goes = in: -> {"execute": "query-config-schema", "arguments" : {"option": "drive"}} <- { "return": [ { "params": [ { "name": "bus", "type": "number", "help": "bus number" }, { "name": "if", "type": "string", "help": "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)" }, ... ], "option": "drive" } ] } > =20 > +ConfigSchemaInfoList *qmp_query_config_schema(bool has_option, > + const char *option, Erro= r **errp) > +{ > + ConfigSchemaInfoList *conf_list =3D NULL, *entry; > + ConfigSchemaInfo *info; > + strList *str_list =3D NULL, *str_entry; > + int entries, i, j; > + > + entries =3D ARRAY_SIZE(vm_config_groups); > + > + for (i =3D 0; i < entries; i++) { > + if (vm_config_groups[i] !=3D NULL && > + (!has_option || !strcmp(option, vm_config_groups[i]->name)= )) { > + info =3D g_malloc0(sizeof(*info)); This part of the iteration looks fine. > + info->option =3D g_strdup(vm_config_groups[i]->name); > + str_list =3D NULL; > + > + for (j =3D 0; vm_config_groups[i]->desc[j].name !=3D NULL;= j++) { > + str_entry =3D g_malloc0(sizeof(*str_entry)); > + str_entry->value =3D g_strdup(vm_config_groups[i]->des= c[j].name); > + str_entry->next =3D str_list; > + str_list =3D str_entry; > + } Here, you'd need to allocate a bit more structure, to match the JSON I proposed above. > + > + info->params =3D str_list; > + entry =3D g_malloc0(sizeof(*entry)); > + entry->value =3D info; > + entry->next =3D conf_list; > + conf_list =3D entry; > + } > + } > + > + if (conf_list =3D=3D NULL) { > + error_set(errp, QERR_INVALID_OPTION_GROUP, option); > + } > + > + return conf_list; > +} > + > QemuOptsList *qemu_find_opts_err(const char *group, Error **errp) > { > return find_list(vm_config_groups, group, errp); >=20 Looking forward to v2. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2OFRQJVHQXMLSNTWLLBKA 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/ iQEcBAEBCAAGBQJRd+5hAAoJEKeha0olJ0NqrW0H/1aZ6OxHRRwuUj1nuxIbVRqj NkWrXldhXFyGrmKvYymOVTcX+6PRNEqwctKf3eAuzbP2a/TpaDjaMas6hIVnSso7 75C8rwLJckgVMs59YxPsvIn6xkp/1nYh7WTFE2ei3V6SPjjqMROxD7/Z48Yc5vNG YhIam95Px4HsaipW6jHPWwA3/nEFUpILPPJIR5jVU9eDS+YciReo3vehq0dcOQrm J9kqyVrW0vUgBoC13GtekLdutGKTXNFRoEWZ6mgkxHWoWAPufOkrHwavG9AMhYwH vTaf19Z72LVw+FgfXo8VH44cJ1wY/Y9hHIo35FHLLToMM/ydmZyNUloVjn6bphQ= =T9M0 -----END PGP SIGNATURE----- ------enig2OFRQJVHQXMLSNTWLLBKA--