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

[Eric Blake]

On 05/04/2018 03:37 AM, Igor Mammedov wrote:

Subject line is stale, needs to be updated to match new spelling...

> 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 amended to not expect machine
> 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

another stale comment...

> 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>
> ---
> v7:
>    - (Eric Blake <address@hidden>)
>      * s/allowed-in-preconfig/allow-preconfig/

...used in the rest of the patch.

>      * s/allowed_in_preconfig/allow_preconfig/
>      * move here QCO_ALLOWED_IN_PRECONFIG declaration from
>         'cli: add --preconfig option'
>        and put this patch before it as well
>      * s/QCO_ALLOWED_IN_PRECONFIG/QCO_ALLOW_PRECONFIG/
>      * wording fixes in doc

> +++ b/include/qapi/qmp/dispatch.h
> @@ -23,6 +23,7 @@ typedef enum QmpCommandOptions
>       QCO_NO_OPTIONS            =  0x0,
>       QCO_NO_SUCCESS_RESP       =  (1U << 0),
>       QCO_ALLOW_OOB             =  (1U << 1),
> +    QCO_ALLOW_PRECONFIG       =  (1U << 2),
>   } QmpCommandOptions;
>   
>   typedef struct QmpCommand
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index a569d24..0f9fbea 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ 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, '*allow-preconfig': true }

Thanks for taking on the bikeshed renaming; it does look better now that 
the two flags have similar spellings.

>   
>   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 'allow-preconfig' key to permit its execution

s/use/use the/

> +at early runtime configuration stage (preconfig runstate).
> +If not specified then a command defaults to 'allow-preconfig': false

trailing '.'

> +
> +An example of declaring a command that is enabled during preconfig:
> + { 'command': 'qmp_capabilities',
> +   'allow-preconfig': true }

It might be worth having this example match our current schema, as in:

{ 'command': 'qmp_capabilities',
  'allow-preconfig': true,
  'data': { '*enable': [ 'QMPCapability' ] } }

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

Or the way you actually wrote it ;)

Otherwise looks pretty good.

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