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
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 <imammedo@redhat.com>
---
v6:
  * exclude 'cont' command from preconfig enabled, in favor of
    exit-preconfig command
  * makr exit-preconfig with allowed-in-preconfig=true
v5:
  * allow query-command-line-options in preconfig state
  * rebase on top of OOB changes that's now in master
  * spelling/wording fixups
  * make sure that allowed-in-preconfig could be set only to True
  * move out QCO_ALLOWED_IN_PRECONFIG check in do_qmp_dispatch() to
    earlier 'cli: add -preconfig option' patch
v4:
  * replaces complex "universal" approach
     "[PATCH v3 5/9] QAPI: allow to specify valid runstates  per command"
    with a simpler new command flag "allowed-in-preconfig".
    (Eric Blake <eblake@redhat.com>)
---
 docs/devel/qapi-code-gen.txt   | 10 +++++++++-
 monitor.c                      |  5 +++--
 qapi/introspect.json           |  5 ++++-
 qapi/misc.json                 | 11 +++++++----
 qapi/run-state.json            |  3 ++-
 scripts/qapi/commands.py       | 12 ++++++++----
 scripts/qapi/common.py         | 19 ++++++++++++-------
 scripts/qapi/doc.py            |  4 ++--
 scripts/qapi/introspect.py     |  7 ++++---
 tests/qapi-schema/test-qapi.py |  4 ++--
 10 files changed, 53 insertions(+), 27 deletions(-)

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index a569d24..31c4646 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, '*allowed-in-preconfig': true }
 
 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'.
+
+An example of declaring preconfig enabled command:
+ { '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);
     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);
 }
 
 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)
+#
 # TODO: @success-response (currently irrelevant, because it's QGA, not QMP)
 #
 # Since: 2.5
 ##
 { 'struct': 'SchemaInfoCommand',
   'data': { 'arg-type': 'str', 'ret-type': 'str',
-            'allow-oob': 'bool' } }
+            'allow-oob': 'bool', 'allowed-in-preconfig': 'bool' } }
 
 ##
 # @SchemaInfoEvent:
diff --git a/qapi/misc.json b/qapi/misc.json
index dfd390b..cbe8338 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -35,7 +35,8 @@
 #
 ##
 { 'command': 'qmp_capabilities',
-  'data': { '*enable': [ 'QMPCapability' ] } }
+  'data': { '*enable': [ 'QMPCapability' ] },
+  'allowed-in-preconfig': true }
 
 ##
 # @QMPCapability:
@@ -153,7 +154,8 @@
 # Note: This example has been shortened as the real response is too long.
 #
 ##
-{ 'command': 'query-commands', 'returns': ['CommandInfo'] }
+{ 'command': 'query-commands', 'returns': ['CommandInfo'],
+  'allowed-in-preconfig': true }
 
 ##
 # @LostTickPolicy:
@@ -1228,7 +1230,7 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'exit-preconfig' }
+{ 'command': 'exit-preconfig', 'allowed-in-preconfig': true }
 
 ##
 # @system_wakeup:
@@ -2636,7 +2638,8 @@
 #
 ##
 {'command': 'query-command-line-options', 'data': { '*option': 'str' },
- 'returns': ['CommandLineOptionInfo'] }
+ 'returns': ['CommandLineOptionInfo'],
+ 'allowed-in-preconfig': true }
 
 ##
 # @X86CPURegister32:
diff --git a/qapi/run-state.json b/qapi/run-state.json
index 9694a9f..d6ed5d8 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -94,7 +94,8 @@
 #                  "status": "running" } }
 #
 ##
-{ 'command': 'query-status', 'returns': 'StatusInfo' }
+{ 'command': 'query-status', 'returns': 'StatusInfo',
+  'allowed-in-preconfig': true }
 
 ##
 # @SHUTDOWN:
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 0c5da3a..9975d95 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -193,13 +193,16 @@ out:
     return ret
 
 
-def gen_register_command(name, success_response, allow_oob):
+def gen_register_command(name, success_response, allow_oob,
+                         allowed_in_preconfig):
     options = []
 
     if not success_response:
         options += ['QCO_NO_SUCCESS_RESP']
     if allow_oob:
         options += ['QCO_ALLOW_OOB']
+    if allowed_in_preconfig:
+        options += ['QCO_ALLOWED_IN_PRECONFIG']
 
     if not options:
         options = ['QCO_NO_OPTIONS']
@@ -275,8 +278,8 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
                        c_prefix=c_name(self._prefix, protect=False)))
         genc.add(gen_registry(self._regy, self._prefix))
 
-    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):
         if not gen:
             return
         self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
@@ -285,7 +288,8 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
             self._genc.add(gen_marshal_output(ret_type))
         self._genh.add(gen_marshal_decl(name))
         self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
-        self._regy += gen_register_command(name, success_response, allow_oob)
+        self._regy += gen_register_command(name, success_response, allow_oob,
+                                           allowed_in_preconfig)
 
 
 def gen_commands(schema, output_dir, prefix):
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 3e14bc4..87273a2 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -872,7 +872,8 @@ def check_keys(expr_elem, meta, required, optional=[]):
             raise QAPISemError(info,
                                "'%s' of %s '%s' should only use false value"
                                % (key, meta, name))
-        if (key == 'boxed' or key == 'allow-oob') and value is not True:
+        if (key == 'boxed' or key == 'allow-oob' or
+            key == 'allowed-in-preconfig') and value is not True:
             raise QAPISemError(info,
                                "'%s' of %s '%s' should only use true value"
                                % (key, meta, name))
@@ -922,7 +923,7 @@ def check_exprs(exprs):
             meta = 'command'
             check_keys(expr_elem, 'command', [],
                        ['data', 'returns', 'gen', 'success-response',
-                        'boxed', 'allow-oob'])
+                        'boxed', 'allow-oob', 'allowed-in-preconfig'])
         elif 'event' in expr:
             meta = 'event'
             check_keys(expr_elem, 'event', [], ['data', 'boxed'])
@@ -1044,8 +1045,8 @@ class QAPISchemaVisitor(object):
     def visit_alternate_type(self, name, info, variants):
         pass
 
-    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):
         pass
 
     def visit_event(self, name, info, arg_type, boxed):
@@ -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):
         QAPISchemaEntity.__init__(self, name, info, doc)
         assert not arg_type or isinstance(arg_type, str)
         assert not ret_type or isinstance(ret_type, str)
@@ -1434,6 +1435,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
         self.success_response = success_response
         self.boxed = boxed
         self.allow_oob = allow_oob
+        self.allowed_in_preconfig = allowed_in_preconfig
 
     def check(self, schema):
         if self._arg_type_name:
@@ -1458,7 +1460,8 @@ class QAPISchemaCommand(QAPISchemaEntity):
         visitor.visit_command(self.name, self.info,
                               self.arg_type, self.ret_type,
                               self.gen, self.success_response,
-                              self.boxed, self.allow_oob)
+                              self.boxed, self.allow_oob,
+                              self.allowed_in_preconfig)
 
 
 class QAPISchemaEvent(QAPISchemaEntity):
@@ -1678,6 +1681,7 @@ class QAPISchema(object):
         success_response = expr.get('success-response', True)
         boxed = expr.get('boxed', False)
         allow_oob = expr.get('allow-oob', False)
+        allowed_in_preconfig = expr.get('allowed-in-preconfig', False)
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
                 name, info, doc, 'arg', self._make_members(data, info))
@@ -1686,7 +1690,8 @@ class QAPISchema(object):
             rets = self._make_array_type(rets[0], info)
         self._def_entity(QAPISchemaCommand(name, info, doc, data, rets,
                                            gen, success_response,
-                                           boxed, allow_oob))
+                                           boxed, allow_oob,
+                                           allowed_in_preconfig))
 
     def _def_event(self, expr, info, doc):
         name = expr['event']
diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
index 9b312b2..cb6f2e1 100644
--- a/scripts/qapi/doc.py
+++ 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)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index f9e67e8..5246be1 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -171,14 +171,15 @@ const QLitObject %(c_name)s = %(c_string)s;
                        {'members': [{'type': self._use_type(m.type)}
                                     for m in variants.variants]})
 
-    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):
         arg_type = arg_type or self._schema.the_empty_object_type
         ret_type = ret_type or self._schema.the_empty_object_type
         self._gen_qlit(name, 'command',
                        {'arg-type': self._use_type(arg_type),
                         'ret-type': self._use_type(ret_type),
-                        'allow-oob': allow_oob})
+                        'allow-oob': allow_oob,
+                        'allowed-in-preconfig': allowed_in_preconfig})
 
     def visit_event(self, name, info, arg_type, boxed):
         arg_type = arg_type or self._schema.the_empty_object_type
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index c1a144b..73d2c77 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -41,8 +41,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
         print('alternate %s' % name)
         self._print_variants(variants)
 
-    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):
         print('command %s %s -> %s' % \
               (name, arg_type and arg_type.name, ret_type and ret_type.name))
         print('   gen=%s success_response=%s boxed=%s oob=%s' % \
-- 
2.7.4

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 <imammedo@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).

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

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

On 04/27/2018 10:51 PM, Peter Xu wrote:

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

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

Ah, that makes sense.  I overlooked the difference in list name in the 
first parameter to qmp_register_command(), since that line was not 
changed in the diff.

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

On Fri, 27 Apr 2018 17:05:30 -0500
Eric Blake <eblake@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 <imammedo@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!

On Fri, Apr 27, 2018 at 05:05:17PM +0200, 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
> 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.

Looking at what libvirt sends to QEMU before 'cont', when -S is used,
we have a lot of differences. In order, libvirt does something like
this on guest startup:

"qmp_capabilities"
"query-migrate-capabilities"
"migrate-set-capabilities"
"query-chardev"
"qom-list"
"qom-get"
"qom-get"
"query-hotpluggable-cpus"
"query-cpus"
"query-iothreads"
"balloon"
"query-balloon"
"cont"

So --preconfig looks like it cannot be used for anything libvirt currently
does during guest startup. This would force us to use the new set-numa-node
command right at the start, even though this is possibly not a time when
libvirt is actually ready to set the numa node in QEMU. This kind of code
ordering constraint QEMU will put on libvirt is why I really don't like
the idea of having multiple startup phases where you can only use certain
commands in certain phases.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Tue, 1 May 2018 16:29:10 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Fri, Apr 27, 2018 at 05:05:17PM +0200, 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
> > 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.  
> 
> Looking at what libvirt sends to QEMU before 'cont', when -S is used,
> we have a lot of differences. In order, libvirt does something like
> this on guest startup:
I took a quick look at these commands in QEMU

> "qmp_capabilities"
this is preconfig enabled within this series

> "query-migrate-capabilities"
> "migrate-set-capabilities"
it could be possible to make it work at preconfig time,
but we probably need to fix -incoming mess first
(as David pointed out it's possible to postpone switching to
inmigrate runstate but needs some work done to make it
actually happen)

> "query-chardev"
could be enabled at preconfig with some behavioral change,
namely, frontend-open field would report false at preconfig time

> "qom-list"
> "qom-get"
> "qom-get"
Above depends on whether being queried object exits at
preconfig time, so possibility of using commands should be
evaluated separately for each use case.
Could you point out to relevant objects that libvirt
uses with these commands and why it's needed?

> "query-hotpluggable-cpus"
preconfig enabled within this series, so that
libvirt could get cpus layout for configuring numa
(it should work for x86 and spapr, and later with AR
once CPU hotplug is enabled there)

> "query-iothreads"
both -object and monitor_init_globals() are done before machine_init
so it should be possible make it work at preconfig

> "balloon"
> "query-balloon"
These are depend on initialized machine and -device balloon processed
What's the reasons for libvirt to use it at -S pause instead of
using -device on CLI for adding balloon device?

> "query-cpus"
That one probably won't be possible as it needs initialized machine
or at least 'machine_init() + -device' processed first.
Why libvirt does need it to start VM?

Above 3 commands and possibly others could be made usable
with --preconfig in case we implement steps between
  machine_run_board_init() ... qdev_machine_creation_done()
as qmp commands like: qmp_initialize_board(), enable device_add,
etc. and move earlier the steps from there that don't really
depend on being created machine. 

> "cont"
> 
> So --preconfig looks like it cannot be used for anything libvirt currently
> does during guest startup. This would force us to use the new set-numa-node
> command right at the start, even though this is possibly not a time when
> libvirt is actually ready to set the numa node in QEMU.
set_numa_node at preconfig time was made with libvirt in mind
so this should work, Peter is just waiting on it being merged
before he would coding libvirt support for it.

> This kind of code
> ordering constraint QEMU will put on libvirt is why I really don't like
> the idea of having multiple startup phases where you can only use certain
> commands in certain phases.
I've even implemented fixing up built machine at -S pause point
for the sake of experiment [1] at he cost of an extra machine reset
per every set-numa-node command, but even if it would work for numa,
is really not good approach, rather fragile and won't scale, we would
be just piling one hack on another.
Hopefully we would be able to make enough commands preconfig enabled
to start VM without need for -S.

> Regards,
> Daniel
[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg502794.html