On Fri, 27 Apr 2018 17:05:30 -0500
Eric Blake <ebl...@redhat.com> wrote:
> 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 <imamm...@redhat.com>
> > ---
> > 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).
Since I'll need to respin anyways, lets got with shorter name
>
> >
> > 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)
yep, it's pretty much copypast.
Could you point to a place/example commit that adds documentation?
> 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).
I'll fix the rest of comments and reshuffle series as you suggest here
and in previous review comments.
Thanks!
