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

[Peter Xu]

On Fri, Apr 27, 2018 at 05:05:30PM -0500, Eric Blake wrote:

[...]

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

[1]

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

My understanding is that we have two lists:

    QmpCommandList qmp_commands, qmp_cap_negotiation_commands;

And here it only registers with qmp_commands list via:

    qmp_init_marshal(&qmp_commands);

But not for the other one, which is explicitly registered at [1].  So
it seems that [1] is still needed?

Thanks,

-- 
Peter Xu