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

[Igor Mammedov]

On Fri, 11 May 2018 10:26:45 -0500
Eric Blake <address@hidden> wrote:

> 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.

There was no reason to refuse good suggestion.


I've just posted fixed up v8 of this patch as reply here,
since changes don't affect other patches.

> 
> >   
> >   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.
> 
Thanks for reviewing!