Re: [Qemu-devel] [PATCH v6 05/11] qapi: introduce new cmd option "allowed-in-preconfig"

[Eric Blake]

On 04/27/2018 10:05 AM, Igor Mammedov wrote:
> New option will be used to allow commands, which are prepared/need
> to run, during preconfig state. Other commands that should be able
> to run in preconfig state, should be ameded to not expect machine

s/ameded/amended/

> in initialized state or deal with it.
> 
> For compatibility reasons, commands that don't use new flag
> 'allowed-in-preconfig' explicitly are not permitted to run in
> preconfig state but allowed in all other states like they used
> to be.
> 
> Within this patch allow following commands in preconfig state:
>    qmp_capabilities
>    query-qmp-schema
>    query-commands
>    query-command-line-options
>    query-status
>    exit-preconfig
> to allow qmp connection, basic introspection and moving to the next
> state.
> 
> PS:
> set-numa-node and query-hotpluggable-cpus will be enabled later in
> a separate patches.
> 
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> v6:
>   * exclude 'cont' command from preconfig enabled, in favor of
>     exit-preconfig command
>   * makr exit-preconfig with allowed-in-preconfig=true

s/makr/mark/ (not that the changelog matters to git...)


> +++ b/docs/devel/qapi-code-gen.txt
> @@ -559,7 +559,7 @@ following example objects:
>  Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
>           '*returns': TYPE-NAME, '*boxed': true,
>           '*gen': false, '*success-response': false,
> -         '*allow-oob': true }
> +         '*allow-oob': true, '*allowed-in-preconfig': true }

Bikeshedding - is it worth naming this just 'allow-preconfig', for a bit
more similarity to 'allow-oob'?  It's less typing, and still conveys the
same amount of information (allow preconfig to use this command).

>  
>  Commands are defined by using a dictionary containing several members,
>  where three members are most common.  The 'command' member is a
> @@ -683,6 +683,14 @@ OOB command handlers must satisfy the following 
> conditions:
>  
>  If in doubt, do not implement OOB execution support.
>  
> +A command may use optional 'allowed-in-preconfig' key to permit
> +its execution at early runtime configuration stage (preconfig runstate).
> +If not specified then a command defaults to 'allowed-in-preconfig: false'.

Unusual spelling for JSON:
 s/'allowed-in-preconfig: false'/'allowed-in-preconfig':false/

and of course, additional tweaks here and in other patches if you like
my bikeshedding for a shorter name.

> +
> +An example of declaring preconfig enabled command:

Might read better as:
An example of declaring a command that is enabled during preconfig:

> + { 'command': 'qmp_capabilities',
> +   'allowed-in-preconfig': true }
> +
>  === Events ===
>  
>  Usage: { 'event': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
> diff --git a/monitor.c b/monitor.c
> index 0ffdf1d..e5e60dc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1183,7 +1183,7 @@ static void monitor_init_qmp_commands(void)
>  
>      qmp_register_command(&qmp_commands, "query-qmp-schema",
>                           qmp_query_qmp_schema,
> -                         QCO_NO_OPTIONS);
> +                         QCO_ALLOWED_IN_PRECONFIG);

I understand why query-qmp-schema is special cased (because it uses
'gen':false in QAPI), but...

>      qmp_register_command(&qmp_commands, "device_add", qmp_device_add,
>                           QCO_NO_OPTIONS);
>      qmp_register_command(&qmp_commands, "netdev_add", qmp_netdev_add,
> @@ -1193,7 +1193,8 @@ static void monitor_init_qmp_commands(void)
>  
>      QTAILQ_INIT(&qmp_cap_negotiation_commands);
>      qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities",
> -                         qmp_marshal_qmp_capabilities, QCO_NO_OPTIONS);
> +                         qmp_marshal_qmp_capabilities,
> +                         QCO_ALLOWED_IN_PRECONFIG);

...why are we still special-casing the registration of qmp_capabilities
here...

>  }
>  
>  static bool qmp_cap_enabled(Monitor *mon, QMPCapability cap)
> diff --git a/qapi/introspect.json b/qapi/introspect.json
> index c7f67b7..8036154 100644
> --- a/qapi/introspect.json
> +++ b/qapi/introspect.json
> @@ -262,13 +262,16 @@
>  # @allow-oob: whether the command allows out-of-band execution.
>  #             (Since: 2.12)
>  #
> +# @allowed-in-preconfig: command can be executed in preconfig runstate,
> +#                        default: 'false' (Since 2.13)
> +#

If we like the bikeshedding on the QAPI spelling, I think it also
applies to the introspection spelling.


> +++ b/qapi/misc.json
> @@ -35,7 +35,8 @@
>  #
>  ##
>  { 'command': 'qmp_capabilities',
> -  'data': { '*enable': [ 'QMPCapability' ] } }
> +  'data': { '*enable': [ 'QMPCapability' ] },
> +  'allowed-in-preconfig': true }

...should't this be good enough to get qmp_capabilities registered?  Hmm
- maybe that's an independent cleanup (and it might be missed fallout
from Peter Xu's OOB work).


> +++ b/scripts/qapi/common.py

> @@ -1422,7 +1423,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
>  
>  class QAPISchemaCommand(QAPISchemaEntity):
>      def __init__(self, name, info, doc, arg_type, ret_type,
> -                 gen, success_response, boxed, allow_oob):
> +                 gen, success_response, boxed, allow_oob, 
> allowed_in_preconfig):

Borderline long line; I'd wrap it (except that if you use the shorter
name allow_preconfig, then there's not a long line issue).

> +++ b/scripts/qapi/doc.py
> @@ -226,8 +226,8 @@ class 
> QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
>                                 name=doc.symbol,
>                                 body=texi_entity(doc, 'Members')))
>  
> -    def visit_command(self, name, info, arg_type, ret_type,
> -                      gen, success_response, boxed, allow_oob):
> +    def visit_command(self, name, info, arg_type, ret_type, gen,
> +                      success_response, boxed, allow_oob, 
> allowed_in_preconfig):
>          doc = self.cur_doc
>          if boxed:
>              body = texi_body(doc)

It's nice that introspection covers whether a command is allowed during
preconfig; but wouldn't it also be nice if the documentation also
mentioned this attribute?  (Hmm, partly my fault for missing during
review of OOB that the same can be said about documentation of allow_oob
- so you were just copying existing practice)

Overall looks reasonable to me, but see my notes elsewhere in the series
about hoisting this earlier in the series while still keeping things
compiling (perhaps by splitting the introduction of
QCO_ALLOWED_IN_PRECONFIG from the introduction of --preconfig on the
command line).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature