From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55218) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UTBiH-0002FS-Vj for qemu-devel@nongnu.org; Fri, 19 Apr 2013 09:50:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UTBiG-0000F5-7w for qemu-devel@nongnu.org; Fri, 19 Apr 2013 09:50:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37064) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UTBiF-0000Ev-V5 for qemu-devel@nongnu.org; Fri, 19 Apr 2013 09:50:52 -0400 Message-ID: <51714BB1.1010102@redhat.com> Date: Fri, 19 Apr 2013 21:50:41 +0800 From: Osier Yang MIME-Version: 1.0 References: <1366365123-5412-1-git-send-email-akong@redhat.com> <51713B08.1050504@redhat.com> In-Reply-To: <51713B08.1050504@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: lcapitulino@redhat.com, aliguori@us.ibm.com, Amos Kong , qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com On 19/04/13 20:39, Eric Blake wrote: > On 04/19/2013 03:52 AM, Amos Kong wrote: >> Libvirt doesn't have a stable way to know option support >> detail. This patch introdued a new qmp command to query >> configuration schema information. hmp command isn't added. > Agreed; HMP is not needed: by the time you can connect to a human > monitor, you've already invoked the command line, and humans can read > -help. It's just libvirt (and other management apps) that need a > machine-parseable alternative to -help. > >> Signed-off-by: Amos Kong >> CC: Osier Yang >> CC: Anthony Liguori >> --- >> Reposted this patch: https://www.redhat.com/archives/libvir-list/2013-January/msg01656.html >> --- >> qapi-schema.json | 26 ++++++++++++++++++++++++++ >> qemu-options-wrapper.h | 16 ++++++++++++++++ >> qmp-commands.hx | 32 ++++++++++++++++++++++++++++++++ >> qmp.c | 36 ++++++++++++++++++++++++++++++++++++ >> 4 files changed, 110 insertions(+), 0 deletions(-) > In addition to Osier's review, > >> +## >> +# @ConfigSchemaInfo: >> +# >> +# Configration schema information. >> +# >> +# @option: option name >> +# >> +# @config-schema: configuration schema string of one option > Any more details about the typical contents of this string? Is it > supposed to be machine-parseable, or is it merely a human-readable > replay of -help text where the only machine-parseable aspect is whether > a particular @option name exists? > >> +# >> +# Since 1.5 >> +## >> +{ 'type': 'ConfigSchemaInfo', 'data': {'option': 'str', 'config-schema': 'str'} } >> + >> +## >> +# @query-config-schema >> +# >> +# Query configuration schema information of options >> +# >> +# @option: #optional option name > I don't know that may existing query-* commands support optional > arguments for filtering; but it seems like a nice enough thing to provide. > >> +# >> +# Returns: @ConfigSchemaInfo list >> +# >> +# Since 1.5 >> +## >> +{'command': 'query-config-schema', 'data': {'*option': 'str'}, 'returns': ['ConfigSchemaInfo']} > Long line; it might be worth wrapping this similar to what other > commands have done. > >> +SQMP >> +query-config-schema >> +------------ >> + >> +Show configuration schema. >> + >> +Return configuration schema of one option, if no option is assigned, >> +will return configuration schema of all options. >> + >> +- "option": option name >> +- "config-schema": configuration schema string of one option >> + >> +Example: >> +-> {"execute": "query-config-schema", "arguments" : {"option": "boot"}} >> +<- {"return": [ >> + {"config-schema": "-boot [order=drives][,once=drives][,menu=on|off]\n >> + [,splash=sp_name][,splash-time=sp_time][,reboot-timeout=rb_time][,strict=on|off]\n >> + 'drives': floppy (a), hard disk (c), CD-ROM (d), network (n)\n >> + 'sp_name': the file's name that would be passed to bios as logo picture, if menu=on\n >> + 'sp_time': the period that splash picture last if menu=on, unit is ms\n >> + 'rb_timeout': the timeout before guest reboot when boot failed, unit is ms\n", > Ah, the example makes it clear - this is human-readable text from -help > output, and not something that is further machine-parseable. Which > means that if we have an existing command line version in one qemu > release, and the next qemu release adds more features to that option, we > have no reliable way (other than string-scraping) to detect that new > feature of the existing option. Can we do any better? > > I was expecting more of a JSON representation of the > QEMUOptionParameter[] used to parse a given option - THAT would be > machine-parseable, and would also let us know when an option has learned > more arguments in a newer qemu version. Agreed. Parsing with the string is not a happy work. Though the existing code in libvirt can be reused. > But it is also something that > can be added later (we've already decided that query-* commands can add > output fields to their dictionary as needed). I'm wondering if can have a JSON dictionary from the beginning. > > The name 'config-schema' sounds like it is machine-parseable; can we use > a more descriptive name such as 'help-text' that makes it obvious what > you are really providing? > >> +++ b/qmp.c >> @@ -24,6 +24,9 @@ >> #include "hw/qdev.h" >> #include "sysemu/blockdev.h" >> #include "qom/qom-qobject.h" >> +#include "qemu-options.h" >> +#include "net/net.h" > What does net/net.h have to do with anything? It's required for building, see the two #includes of qemu-options-wrapper.h Osier