[Qemu-devel] [PATCH v3 0/7] Reenable hmp for preconfig mode

[Qemu-devel] [PATCH v3 0/7] Reenable hmp for preconfig mode

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Reenable HMP in preconfig mode; it's pretty
easy and anyway I want to do a similar thing for OOB eventually.
We'll want to enable more commands in preconfig mode to make it
useful at some point.

Dave

v3
  Add Igor's patch to enable most of the same info commands in qmp

Dr. David Alan Gilbert (6):
  hmp: Add flag for preconfig commands
  hmp: Allow help on preconfig commands
  hmp: Restrict auto-complete in preconfig
  hmp: Add info commands for preconfig
  hmp: add exit_preconfig
  hmp: Allow HMP in preconfig state again

Igor Mammedov (1):
  qmp: enable query-[chardev|version|name|uuid|iothreads|memdev]
    commands in preconfig state

 hmp-commands-info.hx | 12 ++++++++++++
 hmp-commands.hx      | 17 +++++++++++++++++
 hmp.c                |  7 +++++++
 hmp.h                |  1 +
 monitor.c            | 43 ++++++++++++++++++++++++++++++++++---------
 qapi/char.json       |  3 ++-
 qapi/misc.json       | 12 +++++++-----
 7 files changed, 80 insertions(+), 15 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH v3 1/7] hmp: Add flag for preconfig commands

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Add a flag to command definitions to allow them to be used in preconfig
and check it.
If users try to use commands that aren't available, tell them to use
the exit_preconfig comand we're adding in a few patches.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 monitor.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/monitor.c b/monitor.c
index 6d0cec552e..f4a16e6a03 100644
--- a/monitor.c
+++ b/monitor.c
@@ -128,6 +128,7 @@ typedef struct mon_cmd_t {
     const char *args_type;
     const char *params;
     const char *help;
+    const char *flags; /* p=preconfig */
     void (*cmd)(Monitor *mon, const QDict *qdict);
     /* @sub_table is a list of 2nd level of commands. If it does not exist,
      * cmd should be used. If it exists, sub_table[?].cmd should be
@@ -936,6 +937,19 @@ static int parse_cmdline(const char *cmdline,
     return -1;
 }
 
+/*
+ * Returns true if the command can be executed in preconfig mode
+ * i.e. it has the 'p' flag.
+ */
+static bool cmd_can_preconfig(const mon_cmd_t *cmd)
+{
+    if (!cmd->flags) {
+        return false;
+    }
+
+    return strchr(cmd->flags, 'p');
+}
+
 static void help_cmd_dump_one(Monitor *mon,
                               const mon_cmd_t *cmd,
                               char **prefix_args,
@@ -2976,6 +2990,12 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                        (int)(p - cmdp_start), cmdp_start);
         return NULL;
     }
+    if (runstate_check(RUN_STATE_PRECONFIG) && !cmd_can_preconfig(cmd)) {
+        monitor_printf(mon, "Command '%.*s' not available with -preconfig; "
+                            "use exit_preconfig after configuration.\n",
+                       (int)(p - cmdp_start), cmdp_start);
+        return NULL;
+    }
 
     /* filter out following useless space */
     while (qemu_isspace(*p)) {
-- 
2.17.0

[Qemu-devel] [PATCH v3 2/7] hmp: Allow help on preconfig commands

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Allow the 'help' command in preconfig state but
make it only list the preconfig commands.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 hmp-commands.hx | 1 +
 monitor.c       | 8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 0734fea931..8bf590ae4b 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -15,6 +15,7 @@ ETEXI
         .params     = "[cmd]",
         .help       = "show the help",
         .cmd        = do_help_cmd,
+        .flags      = "p",
     },
 
 STEXI
diff --git a/monitor.c b/monitor.c
index f4a16e6a03..31c8f5dc88 100644
--- a/monitor.c
+++ b/monitor.c
@@ -957,6 +957,10 @@ static void help_cmd_dump_one(Monitor *mon,
 {
     int i;
 
+    if (runstate_check(RUN_STATE_PRECONFIG) && !cmd_can_preconfig(cmd)) {
+        return;
+    }
+
     for (i = 0; i < prefix_args_nb; i++) {
         monitor_printf(mon, "%s ", prefix_args[i]);
     }
@@ -979,7 +983,9 @@ static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
 
     /* Find one entry to dump */
     for (cmd = cmds; cmd->name != NULL; cmd++) {
-        if (compare_cmd(args[arg_index], cmd->name)) {
+        if (compare_cmd(args[arg_index], cmd->name) &&
+            ((!runstate_check(RUN_STATE_PRECONFIG) ||
+                cmd_can_preconfig(cmd)))) {
             if (cmd->sub_table) {
                 /* continue with next arg */
                 help_cmd_dump(mon, cmd->sub_table,
-- 
2.17.0

[Qemu-devel] [PATCH v3 3/7] hmp: Restrict auto-complete in preconfig

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Don't show the commands that aren't available.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 monitor.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index 31c8f5dc88..c369b392db 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3951,12 +3951,17 @@ static void monitor_find_completion_by_table(Monitor *mon,
             cmdname = args[0];
         readline_set_completion_index(mon->rs, strlen(cmdname));
         for (cmd = cmd_table; cmd->name != NULL; cmd++) {
-            cmd_completion(mon, cmdname, cmd->name);
+            if (!runstate_check(RUN_STATE_PRECONFIG) ||
+                 cmd_can_preconfig(cmd)) {
+                cmd_completion(mon, cmdname, cmd->name);
+            }
         }
     } else {
         /* find the command */
         for (cmd = cmd_table; cmd->name != NULL; cmd++) {
-            if (compare_cmd(args[0], cmd->name)) {
+            if (compare_cmd(args[0], cmd->name) &&
+                (!runstate_check(RUN_STATE_PRECONFIG) ||
+                 cmd_can_preconfig(cmd))) {
                 break;
             }
         }
-- 
2.17.0

[Qemu-devel] [PATCH v3 4/7] qmp: enable query-[chardev|version|name|uuid|iothreads|memdev] commands in preconfig state

[Dr. David Alan Gilbert (git)]

From: Igor Mammedov <imammedo@redhat.com>

subj commands, are informational and do not depend on machine being
initialized. Make them available early in preconfig runstate to make
the later a little bit more useful.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 qapi/char.json |  3 ++-
 qapi/misc.json | 12 +++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/qapi/char.json b/qapi/char.json
index ae19dcd1ed..60f31d83fc 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -62,7 +62,8 @@
 #    }
 #
 ##
-{ 'command': 'query-chardev', 'returns': ['ChardevInfo'] }
+{ 'command': 'query-chardev', 'returns': ['ChardevInfo'],
+  'allow-preconfig': true }
 
 ##
 # @ChardevBackendInfo:
diff --git a/qapi/misc.json b/qapi/misc.json
index f83a63a0ab..1be1728c0e 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -117,7 +117,8 @@
 #    }
 #
 ##
-{ 'command': 'query-version', 'returns': 'VersionInfo' }
+{ 'command': 'query-version', 'returns': 'VersionInfo',
+  'allow-preconfig': true }
 
 ##
 # @CommandInfo:
@@ -241,7 +242,7 @@
 # <- { "return": { "name": "qemu-name" } }
 #
 ##
-{ 'command': 'query-name', 'returns': 'NameInfo' }
+{ 'command': 'query-name', 'returns': 'NameInfo', 'allow-preconfig': true }
 
 ##
 # @KvmInfo:
@@ -301,7 +302,7 @@
 # <- { "return": { "UUID": "550e8400-e29b-41d4-a716-446655440000" } }
 #
 ##
-{ 'command': 'query-uuid', 'returns': 'UuidInfo' }
+{ 'command': 'query-uuid', 'returns': 'UuidInfo', 'allow-preconfig': true }
 
 ##
 # @EventInfo:
@@ -710,7 +711,8 @@
 #    }
 #
 ##
-{ 'command': 'query-iothreads', 'returns': ['IOThreadInfo'] }
+{ 'command': 'query-iothreads', 'returns': ['IOThreadInfo'],
+  'allow-preconfig': true }
 
 ##
 # @BalloonInfo:
@@ -2902,7 +2904,7 @@
 #    }
 #
 ##
-{ 'command': 'query-memdev', 'returns': ['Memdev'] }
+{ 'command': 'query-memdev', 'returns': ['Memdev'], 'allow-preconfig': true }
 
 ##
 # @PCDIMMDeviceInfo:
-- 
2.17.0

[Qemu-devel] [PATCH v3 5/7] hmp: Add info commands for preconfig

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Allow a bunch of the info commands to be used in preconfig.

version, chardev, name, uuid,memdev, iothreads
  Were enabled in QMP in the previous patch from Igor

status, hotpluggable_cpus
  Was enabled in the original allow-preconfig series

history
  is HMP specific

usbhost, qom-tree, numa
  Don't have a QMP equivalent

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hmp-commands-info.hx | 12 ++++++++++++
 hmp-commands.hx      |  1 +
 2 files changed, 13 insertions(+)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index ddfcd5adcc..f4d0d7d2d0 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -19,6 +19,7 @@ ETEXI
         .params     = "",
         .help       = "show the version of QEMU",
         .cmd        = hmp_info_version,
+        .flags      = "p",
     },
 
 STEXI
@@ -47,6 +48,7 @@ ETEXI
         .params     = "",
         .help       = "show the character devices",
         .cmd        = hmp_info_chardev,
+        .flags      = "p",
     },
 
 STEXI
@@ -165,6 +167,7 @@ ETEXI
         .params     = "",
         .help       = "show the command line history",
         .cmd        = hmp_info_history,
+        .flags      = "p",
     },
 
 STEXI
@@ -315,6 +318,7 @@ ETEXI
         .params     = "",
         .help       = "show NUMA information",
         .cmd        = hmp_info_numa,
+        .flags      = "p",
     },
 
 STEXI
@@ -343,6 +347,7 @@ ETEXI
         .params     = "",
         .help       = "show host USB devices",
         .cmd        = hmp_info_usbhost,
+        .flags      = "p",
     },
 
 STEXI
@@ -399,6 +404,7 @@ ETEXI
         .params     = "",
         .help       = "show the current VM status (running|paused)",
         .cmd        = hmp_info_status,
+        .flags      = "p",
     },
 
 STEXI
@@ -457,6 +463,7 @@ ETEXI
         .params     = "",
         .help       = "show the current VM name",
         .cmd        = hmp_info_name,
+        .flags      = "p",
     },
 
 STEXI
@@ -471,6 +478,7 @@ ETEXI
         .params     = "",
         .help       = "show the current VM UUID",
         .cmd        = hmp_info_uuid,
+        .flags      = "p",
     },
 
 STEXI
@@ -613,6 +621,7 @@ ETEXI
         .params     = "[path]",
         .help       = "show QOM composition tree",
         .cmd        = hmp_info_qom_tree,
+        .flags      = "p",
     },
 
 STEXI
@@ -671,6 +680,7 @@ ETEXI
         .params     = "",
         .help       = "show memory backends",
         .cmd        = hmp_info_memdev,
+        .flags      = "p",
     },
 
 STEXI
@@ -699,6 +709,7 @@ ETEXI
         .params     = "",
         .help       = "show iothreads",
         .cmd        = hmp_info_iothreads,
+        .flags      = "p",
     },
 
 STEXI
@@ -829,6 +840,7 @@ ETEXI
         .params     = "",
         .help       = "Show information about hotpluggable CPUs",
         .cmd        = hmp_hotpluggable_cpus,
+        .flags      = "p",
     },
 
 STEXI
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 8bf590ae4b..dc82ed526f 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1856,6 +1856,7 @@ ETEXI
         .help       = "show various information about the system state",
         .cmd        = hmp_info_help,
         .sub_table  = info_cmds,
+        .flags      = "p",
     },
 
 STEXI
-- 
2.17.0

[Qemu-devel] [PATCH v3 6/7] hmp: add exit_preconfig

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Add the exit_preconfig command to return to normality.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 hmp-commands.hx | 15 +++++++++++++++
 hmp.c           |  7 +++++++
 hmp.h           |  1 +
 3 files changed, 23 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index dc82ed526f..40e2f6ca08 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -55,6 +55,21 @@ STEXI
 @item q or quit
 @findex quit
 Quit the emulator.
+ETEXI
+
+    {
+        .name       = "exit_preconfig",
+        .args_type  = "",
+        .params     = "",
+        .help       = "exit the preconfig state",
+        .cmd        = hmp_exit_preconfig,
+        .flags      = "p",
+    },
+
+STEXI
+@item exit_preconfig
+@findex exit_preconfig
+Exit the preconfig state
 ETEXI
 
     {
diff --git a/hmp.c b/hmp.c
index ef93f4878b..c7be6ed394 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1065,6 +1065,13 @@ void hmp_system_powerdown(Monitor *mon, const QDict *qdict)
     qmp_system_powerdown(NULL);
 }
 
+void hmp_exit_preconfig(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    qmp_exit_preconfig(&err);
+    hmp_handle_error(mon, &err);
+}
+
 void hmp_cpu(Monitor *mon, const QDict *qdict)
 {
     int64_t cpu_index;
diff --git a/hmp.h b/hmp.h
index 20f27439d3..33354f1bdd 100644
--- a/hmp.h
+++ b/hmp.h
@@ -44,6 +44,7 @@ void hmp_quit(Monitor *mon, const QDict *qdict);
 void hmp_stop(Monitor *mon, const QDict *qdict);
 void hmp_system_reset(Monitor *mon, const QDict *qdict);
 void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
+void hmp_exit_preconfig(Monitor *mon, const QDict *qdict);
 void hmp_cpu(Monitor *mon, const QDict *qdict);
 void hmp_memsave(Monitor *mon, const QDict *qdict);
 void hmp_pmemsave(Monitor *mon, const QDict *qdict);
-- 
2.17.0

[Qemu-devel] [PATCH v3 7/7] hmp: Allow HMP in preconfig state again

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Now we can cope with preconfig in HMP, reenable by reverting
commit 71dc578e116599ea73c8a2a4e693134702ec0e83.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 monitor.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/monitor.c b/monitor.c
index c369b392db..cd5525e0ec 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3395,12 +3395,6 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
 
     trace_handle_hmp_command(mon, cmdline);
 
-    if (runstate_check(RUN_STATE_PRECONFIG)) {
-        monitor_printf(mon, "HMP not available in preconfig state, "
-                            "use QMP instead\n");
-        return;
-    }
-
     cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table);
     if (!cmd) {
         return;
-- 
2.17.0

Re: [Qemu-devel] [PATCH v3 1/7] hmp: Add flag for preconfig commands

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Add a flag to command definitions to allow them to be used in preconfig
> and check it.
> If users try to use commands that aren't available, tell them to use
> the exit_preconfig comand we're adding in a few patches.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  monitor.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/monitor.c b/monitor.c
> index 6d0cec552e..f4a16e6a03 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -128,6 +128,7 @@ typedef struct mon_cmd_t {
>      const char *args_type;
>      const char *params;
>      const char *help;
> +    const char *flags; /* p=preconfig */

I'd add a bool for each flag instead of encoding them in a string.  No
need for a comment then (yours is cryptic).  But your artistic license
applies.

>      void (*cmd)(Monitor *mon, const QDict *qdict);
>      /* @sub_table is a list of 2nd level of commands. If it does not exist,
>       * cmd should be used. If it exists, sub_table[?].cmd should be
> @@ -936,6 +937,19 @@ static int parse_cmdline(const char *cmdline,
>      return -1;
>  }
>  
> +/*
> + * Returns true if the command can be executed in preconfig mode
> + * i.e. it has the 'p' flag.
> + */
> +static bool cmd_can_preconfig(const mon_cmd_t *cmd)
> +{
> +    if (!cmd->flags) {
> +        return false;
> +    }
> +
> +    return strchr(cmd->flags, 'p');
> +}
> +
>  static void help_cmd_dump_one(Monitor *mon,
>                                const mon_cmd_t *cmd,
>                                char **prefix_args,
> @@ -2976,6 +2990,12 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>                         (int)(p - cmdp_start), cmdp_start);
>          return NULL;
>      }
> +    if (runstate_check(RUN_STATE_PRECONFIG) && !cmd_can_preconfig(cmd)) {
> +        monitor_printf(mon, "Command '%.*s' not available with -preconfig; "
> +                            "use exit_preconfig after configuration.\n",
> +                       (int)(p - cmdp_start), cmdp_start);
> +        return NULL;
> +    }
>  
>      /* filter out following useless space */
>      while (qemu_isspace(*p)) {

Suggest "not available with -preconfig until after exit_preconfig".

Regardless,
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v3 2/7] hmp: Allow help on preconfig commands

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Allow the 'help' command in preconfig state but
> make it only list the preconfig commands.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hmp-commands.hx | 1 +
>  monitor.c       | 8 +++++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 0734fea931..8bf590ae4b 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -15,6 +15,7 @@ ETEXI
>          .params     = "[cmd]",
>          .help       = "show the help",
>          .cmd        = do_help_cmd,
> +        .flags      = "p",
>      },
>  
>  STEXI
> diff --git a/monitor.c b/monitor.c
> index f4a16e6a03..31c8f5dc88 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -957,6 +957,10 @@ static void help_cmd_dump_one(Monitor *mon,
>  {
>      int i;
>  
> +    if (runstate_check(RUN_STATE_PRECONFIG) && !cmd_can_preconfig(cmd)) {
> +        return;
> +    }
> +
>      for (i = 0; i < prefix_args_nb; i++) {
>          monitor_printf(mon, "%s ", prefix_args[i]);
>      }
> @@ -979,7 +983,9 @@ static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
>  
>      /* Find one entry to dump */
>      for (cmd = cmds; cmd->name != NULL; cmd++) {
> -        if (compare_cmd(args[arg_index], cmd->name)) {
> +        if (compare_cmd(args[arg_index], cmd->name) &&
> +            ((!runstate_check(RUN_STATE_PRECONFIG) ||
> +                cmd_can_preconfig(cmd)))) {

Why do we need this check in addition to the one in help_cmd_dump_one()?

>              if (cmd->sub_table) {
>                  /* continue with next arg */
>                  help_cmd_dump(mon, cmd->sub_table,

Re: [Qemu-devel] [PATCH v3 3/7] hmp: Restrict auto-complete in preconfig

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Don't show the commands that aren't available.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  monitor.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 31c8f5dc88..c369b392db 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3951,12 +3951,17 @@ static void monitor_find_completion_by_table(Monitor *mon,
>              cmdname = args[0];
>          readline_set_completion_index(mon->rs, strlen(cmdname));
>          for (cmd = cmd_table; cmd->name != NULL; cmd++) {
> -            cmd_completion(mon, cmdname, cmd->name);
> +            if (!runstate_check(RUN_STATE_PRECONFIG) ||
> +                 cmd_can_preconfig(cmd)) {
> +                cmd_completion(mon, cmdname, cmd->name);
> +            }
>          }
>      } else {
>          /* find the command */
>          for (cmd = cmd_table; cmd->name != NULL; cmd++) {
> -            if (compare_cmd(args[0], cmd->name)) {
> +            if (compare_cmd(args[0], cmd->name) &&
> +                (!runstate_check(RUN_STATE_PRECONFIG) ||
> +                 cmd_can_preconfig(cmd))) {
>                  break;
>              }
>          }

Hmm, I keep seeing

    !runstate_check(RUN_STATE_PRECONFIG) || cmd_can_preconfig(cmd)

Would a helper be worthwhile?  cmd_available(cmd)?

Re: [Qemu-devel] [PATCH v3 2/7] hmp: Allow help on preconfig commands

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Allow the 'help' command in preconfig state but
> > make it only list the preconfig commands.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hmp-commands.hx | 1 +
> >  monitor.c       | 8 +++++++-
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index 0734fea931..8bf590ae4b 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -15,6 +15,7 @@ ETEXI
> >          .params     = "[cmd]",
> >          .help       = "show the help",
> >          .cmd        = do_help_cmd,
> > +        .flags      = "p",
> >      },
> >  
> >  STEXI
> > diff --git a/monitor.c b/monitor.c
> > index f4a16e6a03..31c8f5dc88 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -957,6 +957,10 @@ static void help_cmd_dump_one(Monitor *mon,
> >  {
> >      int i;
> >  
> > +    if (runstate_check(RUN_STATE_PRECONFIG) && !cmd_can_preconfig(cmd)) {
> > +        return;
> > +    }
> > +
> >      for (i = 0; i < prefix_args_nb; i++) {
> >          monitor_printf(mon, "%s ", prefix_args[i]);
> >      }
> > @@ -979,7 +983,9 @@ static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
> >  
> >      /* Find one entry to dump */
> >      for (cmd = cmds; cmd->name != NULL; cmd++) {
> > -        if (compare_cmd(args[arg_index], cmd->name)) {
> > +        if (compare_cmd(args[arg_index], cmd->name) &&
> > +            ((!runstate_check(RUN_STATE_PRECONFIG) ||
> > +                cmd_can_preconfig(cmd)))) {
> 
> Why do we need this check in addition to the one in help_cmd_dump_one()?

To gate entire subtables (we've only currently got 'info' and that's enabled,
anyway, so it never actually triggers).

Dave

> >              if (cmd->sub_table) {
> >                  /* continue with next arg */
> >                  help_cmd_dump(mon, cmd->sub_table,
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 4/7] qmp: enable query-[chardev|version|name|uuid|iothreads|memdev] commands in preconfig state

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: Igor Mammedov <imammedo@redhat.com>
>
> subj commands, are informational and do not depend on machine being
> initialized. Make them available early in preconfig runstate to make
> the later a little bit more useful.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Excessively long subject line.  Suggested fix:

    qmp: Enable a few query- commands in preconfig state

    Commands query-chardev, query-version, query-name, query-uuid,
    query-iothreads, query-memdev are informational and do not depend on
    the machine being initialized.  Make them available in preconfig
    runstate to make the latter a little bit more useful.

> ---
>  qapi/char.json |  3 ++-
>  qapi/misc.json | 12 +++++++-----
>  2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/qapi/char.json b/qapi/char.json
> index ae19dcd1ed..60f31d83fc 100644
> --- a/qapi/char.json
> +++ b/qapi/char.json
> @@ -62,7 +62,8 @@
>  #    }
>  #
>  ##
> -{ 'command': 'query-chardev', 'returns': ['ChardevInfo'] }
> +{ 'command': 'query-chardev', 'returns': ['ChardevInfo'],
> +  'allow-preconfig': true }

Okay because

    if (qemu_opts_foreach(qemu_find_opts("chardev"),
                          chardev_init_func, NULL, NULL)) {
        exit(1);
    }

runs before -preconfig's main_loop().

>  
>  ##
>  # @ChardevBackendInfo:
> diff --git a/qapi/misc.json b/qapi/misc.json
> index f83a63a0ab..1be1728c0e 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -117,7 +117,8 @@
>  #    }
>  #
>  ##
> -{ 'command': 'query-version', 'returns': 'VersionInfo' }
> +{ 'command': 'query-version', 'returns': 'VersionInfo',
> +  'allow-preconfig': true }

Returns data fixed at compile time.  Okay.

>  
>  ##
>  # @CommandInfo:
> @@ -241,7 +242,7 @@
>  # <- { "return": { "name": "qemu-name" } }
>  #
>  ##
> -{ 'command': 'query-name', 'returns': 'NameInfo' }
> +{ 'command': 'query-name', 'returns': 'NameInfo', 'allow-preconfig': true }

Okay because parse_name() runs before -preconfig's main_loop().

>  ##
>  # @KvmInfo:
> @@ -301,7 +302,7 @@
>  # <- { "return": { "UUID": "550e8400-e29b-41d4-a716-446655440000" } }
>  #
>  ##
> -{ 'command': 'query-uuid', 'returns': 'UuidInfo' }
> +{ 'command': 'query-uuid', 'returns': 'UuidInfo', 'allow-preconfig': true }

Okay because qemu_uuid(optarg, &qemu_uuid) runs before -preconfig's
main_loop().

>  
>  ##
>  # @EventInfo:
> @@ -710,7 +711,8 @@
>  #    }
>  #
>  ##
> -{ 'command': 'query-iothreads', 'returns': ['IOThreadInfo'] }
> +{ 'command': 'query-iothreads', 'returns': ['IOThreadInfo'],
> +  'allow-preconfig': true }

Feeling lazy... why is this one okay?

>  
>  ##
>  # @BalloonInfo:
> @@ -2902,7 +2904,7 @@
>  #    }
>  #
>  ##
> -{ 'command': 'query-memdev', 'returns': ['Memdev'] }
> +{ 'command': 'query-memdev', 'returns': ['Memdev'], 'allow-preconfig': true }
>  
>  ##
>  # @PCDIMMDeviceInfo:

And this one?

Re: [Qemu-devel] [PATCH v3 5/7] hmp: Add info commands for preconfig

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Allow a bunch of the info commands to be used in preconfig.
>
> version, chardev, name, uuid,memdev, iothreads
>   Were enabled in QMP in the previous patch from Igor

Yes, these are okay together with PATCH 4.

> status, hotpluggable_cpus
>   Was enabled in the original allow-preconfig series

query-status looks okay to me.

> history
>   is HMP specific

Yes.

> usbhost, qom-tree, numa
>   Don't have a QMP equivalent

HMP commands without a QMP equivalent are okay if their functionality
makes no sense in QMP, or is of use only for human users.

Example for "makes no sense in QMP": setting the current CPU, because a
QMP monitor doesn't have a current CPU.

Examples for "is of use only for human users": HMP command "help", the
integrated pocket calculator.

Now let's review the three commands:

* Gerd, why does "info usbhost" have no QMP equivalent?

* Eduardo, why does "info numa" have no QMP equivalent?

* "info qom-tree" is a recursive variant of qom-list that skips anything
  but children.  This convenience command exists so you don't have to
  filter and string together output from many qom-list.

  I think it stands to reason that if providing "info qom-tree" makes
  sense, then so does qom-list (HMP and QMP).  If qom-list, then
  qom-list-types, qom-list-properties, qom-get, and probably even
  qom-set (I've always been suspicious of qom-set, but that has nothing
  to do with preconfig state).

  It might make sense to split off the whole QOM shebang into a separate
  patch.

> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Re: [Qemu-devel] [PATCH v3 6/7] hmp: add exit_preconfig

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Add the exit_preconfig command to return to normality.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hmp-commands.hx | 15 +++++++++++++++
>  hmp.c           |  7 +++++++
>  hmp.h           |  1 +
>  3 files changed, 23 insertions(+)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index dc82ed526f..40e2f6ca08 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -55,6 +55,21 @@ STEXI
>  @item q or quit
>  @findex quit
>  Quit the emulator.
> +ETEXI
> +
> +    {
> +        .name       = "exit_preconfig",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "exit the preconfig state",
> +        .cmd        = hmp_exit_preconfig,
> +        .flags      = "p",
> +    },
> +
> +STEXI
> +@item exit_preconfig
> +@findex exit_preconfig
> +Exit the preconfig state

This is awfully terse.  Suggest to steal from the QMP documentation in
misc.json:

# This command makes QEMU exit the preconfig state and proceed with
# VM initialization using configuration data provided on the command line
# and via the QMP monitor during the preconfig state. The command is only
# available during the preconfig state (i.e. when the --preconfig command
# line option was in use).

>  ETEXI
>  
>      {
> diff --git a/hmp.c b/hmp.c
> index ef93f4878b..c7be6ed394 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1065,6 +1065,13 @@ void hmp_system_powerdown(Monitor *mon, const QDict *qdict)
>      qmp_system_powerdown(NULL);
>  }
>  
> +void hmp_exit_preconfig(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    qmp_exit_preconfig(&err);
> +    hmp_handle_error(mon, &err);
> +}
> +

Blank line between declaration and statements, please.

>  void hmp_cpu(Monitor *mon, const QDict *qdict)
>  {
>      int64_t cpu_index;
> diff --git a/hmp.h b/hmp.h
> index 20f27439d3..33354f1bdd 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -44,6 +44,7 @@ void hmp_quit(Monitor *mon, const QDict *qdict);
>  void hmp_stop(Monitor *mon, const QDict *qdict);
>  void hmp_system_reset(Monitor *mon, const QDict *qdict);
>  void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
> +void hmp_exit_preconfig(Monitor *mon, const QDict *qdict);
>  void hmp_cpu(Monitor *mon, const QDict *qdict);
>  void hmp_memsave(Monitor *mon, const QDict *qdict);
>  void hmp_pmemsave(Monitor *mon, const QDict *qdict);

Re: [Qemu-devel] [PATCH v3 0/7] Reenable hmp for preconfig mode

[Markus Armbruster]

Let's also provide "quit", both in HMP and QMP.

Re: [Qemu-devel] [PATCH v3 0/7] Reenable hmp for preconfig mode

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Let's also provide "quit", both in HMP and QMP.

I tried enabling quit, but it fails with an incorrect
state transition.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 0/7] Reenable hmp for preconfig mode

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Let's also provide "quit", both in HMP and QMP.
>
> I tried enabling quit, but it fails with an incorrect
> state transition.

Igor, this one's for you.

Re: [Qemu-devel] [PATCH v3 2/7] hmp: Allow help on preconfig commands

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
>> 
>> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> >
>> > Allow the 'help' command in preconfig state but
>> > make it only list the preconfig commands.
>> >
>> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> > Reviewed-by: Peter Xu <peterx@redhat.com>
>> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> > ---
>> >  hmp-commands.hx | 1 +
>> >  monitor.c       | 8 +++++++-
>> >  2 files changed, 8 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/hmp-commands.hx b/hmp-commands.hx
>> > index 0734fea931..8bf590ae4b 100644
>> > --- a/hmp-commands.hx
>> > +++ b/hmp-commands.hx
>> > @@ -15,6 +15,7 @@ ETEXI
>> >          .params     = "[cmd]",
>> >          .help       = "show the help",
>> >          .cmd        = do_help_cmd,
>> > +        .flags      = "p",
>> >      },
>> >  
>> >  STEXI
>> > diff --git a/monitor.c b/monitor.c
>> > index f4a16e6a03..31c8f5dc88 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
>> > @@ -957,6 +957,10 @@ static void help_cmd_dump_one(Monitor *mon,
>> >  {
>> >      int i;
>> >  
>> > +    if (runstate_check(RUN_STATE_PRECONFIG) && !cmd_can_preconfig(cmd)) {
>> > +        return;
>> > +    }
>> > +
>> >      for (i = 0; i < prefix_args_nb; i++) {
>> >          monitor_printf(mon, "%s ", prefix_args[i]);
>> >      }
>> > @@ -979,7 +983,9 @@ static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
>> >  
>> >      /* Find one entry to dump */
>> >      for (cmd = cmds; cmd->name != NULL; cmd++) {
>> > -        if (compare_cmd(args[arg_index], cmd->name)) {
>> > +        if (compare_cmd(args[arg_index], cmd->name) &&
>> > +            ((!runstate_check(RUN_STATE_PRECONFIG) ||
>> > +                cmd_can_preconfig(cmd)))) {
>> 
>> Why do we need this check in addition to the one in help_cmd_dump_one()?
>
> To gate entire subtables (we've only currently got 'info' and that's enabled,
> anyway, so it never actually triggers).

Something's bothering me around here, but I can't put a finger on it...
let me think aloud.

There's an asymmetry between command execution and help.  The former has
just one check, in monitor_parse_command().  If the command has a
sub-table, and there are arguments, monitor_parse_command() recurses
into the sub-table.  Thus, if the command is unavailable, the
sub-commands are unavailable as well, regardless of their "p" flags.

Help's recursion is different.  It's limited to two levels, unlike
execution.  help_cmd_dump_one()'s check applies to the last level.
help_cmd_dump()'s check applies to the first level.  If there's just one
level, we check twice.  Perhaps less than elegant, but harmless and
simple.

You can't make a sub-command available without also making the command
available.  Makes sense, I guess.  If you try, your "p" flags on the
sub-commands are silently ignored.  That's a bit ugly, but if it doesn't
bother you, I can pretend I didn't see it ;)

> Dave
>
>> >              if (cmd->sub_table) {
>> >                  /* continue with next arg */
>> >                  help_cmd_dump(mon, cmd->sub_table,
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 1/7] hmp: Add flag for preconfig commands

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Add a flag to command definitions to allow them to be used in preconfig
> > and check it.
> > If users try to use commands that aren't available, tell them to use
> > the exit_preconfig comand we're adding in a few patches.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  monitor.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 6d0cec552e..f4a16e6a03 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -128,6 +128,7 @@ typedef struct mon_cmd_t {
> >      const char *args_type;
> >      const char *params;
> >      const char *help;
> > +    const char *flags; /* p=preconfig */
> 
> I'd add a bool for each flag instead of encoding them in a string.  No
> need for a comment then (yours is cryptic).  But your artistic license
> applies.

It's a lot smaller change using the string, so simplicity won.

> >      void (*cmd)(Monitor *mon, const QDict *qdict);
> >      /* @sub_table is a list of 2nd level of commands. If it does not exist,
> >       * cmd should be used. If it exists, sub_table[?].cmd should be
> > @@ -936,6 +937,19 @@ static int parse_cmdline(const char *cmdline,
> >      return -1;
> >  }
> >  
> > +/*
> > + * Returns true if the command can be executed in preconfig mode
> > + * i.e. it has the 'p' flag.
> > + */
> > +static bool cmd_can_preconfig(const mon_cmd_t *cmd)
> > +{
> > +    if (!cmd->flags) {
> > +        return false;
> > +    }
> > +
> > +    return strchr(cmd->flags, 'p');
> > +}
> > +
> >  static void help_cmd_dump_one(Monitor *mon,
> >                                const mon_cmd_t *cmd,
> >                                char **prefix_args,
> > @@ -2976,6 +2990,12 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> >                         (int)(p - cmdp_start), cmdp_start);
> >          return NULL;
> >      }
> > +    if (runstate_check(RUN_STATE_PRECONFIG) && !cmd_can_preconfig(cmd)) {
> > +        monitor_printf(mon, "Command '%.*s' not available with -preconfig; "
> > +                            "use exit_preconfig after configuration.\n",
> > +                       (int)(p - cmdp_start), cmdp_start);
> > +        return NULL;
> > +    }
> >  
> >      /* filter out following useless space */
> >      while (qemu_isspace(*p)) {
> 
> Suggest "not available with -preconfig until after exit_preconfig".

Done.

> Regardless,
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 3/7] hmp: Restrict auto-complete in preconfig

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Don't show the commands that aren't available.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  monitor.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 31c8f5dc88..c369b392db 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -3951,12 +3951,17 @@ static void monitor_find_completion_by_table(Monitor *mon,
> >              cmdname = args[0];
> >          readline_set_completion_index(mon->rs, strlen(cmdname));
> >          for (cmd = cmd_table; cmd->name != NULL; cmd++) {
> > -            cmd_completion(mon, cmdname, cmd->name);
> > +            if (!runstate_check(RUN_STATE_PRECONFIG) ||
> > +                 cmd_can_preconfig(cmd)) {
> > +                cmd_completion(mon, cmdname, cmd->name);
> > +            }
> >          }
> >      } else {
> >          /* find the command */
> >          for (cmd = cmd_table; cmd->name != NULL; cmd++) {
> > -            if (compare_cmd(args[0], cmd->name)) {
> > +            if (compare_cmd(args[0], cmd->name) &&
> > +                (!runstate_check(RUN_STATE_PRECONFIG) ||
> > +                 cmd_can_preconfig(cmd))) {
> >                  break;
> >              }
> >          }
> 
> Hmm, I keep seeing
> 
>     !runstate_check(RUN_STATE_PRECONFIG) || cmd_can_preconfig(cmd)
> 
> Would a helper be worthwhile?  cmd_available(cmd)?

Yes, but I want to leave that change until I add the next flag (for OOB
some time) so I know how to generalise it;  I'm not sure what it'll need
yet.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 4/7] qmp: enable query-[chardev|version|name|uuid|iothreads|memdev] commands in preconfig state

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> 
> > From: Igor Mammedov <imammedo@redhat.com>
> >
> > subj commands, are informational and do not depend on machine being
> > initialized. Make them available early in preconfig runstate to make
> > the later a little bit more useful.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Excessively long subject line.  Suggested fix:
> 
>     qmp: Enable a few query- commands in preconfig state
> 
>     Commands query-chardev, query-version, query-name, query-uuid,
>     query-iothreads, query-memdev are informational and do not depend on
>     the machine being initialized.  Make them available in preconfig
>     runstate to make the latter a little bit more useful.
> 
> > ---
> >  qapi/char.json |  3 ++-
> >  qapi/misc.json | 12 +++++++-----
> >  2 files changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/qapi/char.json b/qapi/char.json
> > index ae19dcd1ed..60f31d83fc 100644
> > --- a/qapi/char.json
> > +++ b/qapi/char.json
> > @@ -62,7 +62,8 @@
> >  #    }
> >  #
> >  ##
> > -{ 'command': 'query-chardev', 'returns': ['ChardevInfo'] }
> > +{ 'command': 'query-chardev', 'returns': ['ChardevInfo'],
> > +  'allow-preconfig': true }
> 
> Okay because
> 
>     if (qemu_opts_foreach(qemu_find_opts("chardev"),
>                           chardev_init_func, NULL, NULL)) {
>         exit(1);
>     }
> 
> runs before -preconfig's main_loop().
> 
> >  
> >  ##
> >  # @ChardevBackendInfo:
> > diff --git a/qapi/misc.json b/qapi/misc.json
> > index f83a63a0ab..1be1728c0e 100644
> > --- a/qapi/misc.json
> > +++ b/qapi/misc.json
> > @@ -117,7 +117,8 @@
> >  #    }
> >  #
> >  ##
> > -{ 'command': 'query-version', 'returns': 'VersionInfo' }
> > +{ 'command': 'query-version', 'returns': 'VersionInfo',
> > +  'allow-preconfig': true }
> 
> Returns data fixed at compile time.  Okay.
> 
> >  
> >  ##
> >  # @CommandInfo:
> > @@ -241,7 +242,7 @@
> >  # <- { "return": { "name": "qemu-name" } }
> >  #
> >  ##
> > -{ 'command': 'query-name', 'returns': 'NameInfo' }
> > +{ 'command': 'query-name', 'returns': 'NameInfo', 'allow-preconfig': true }
> 
> Okay because parse_name() runs before -preconfig's main_loop().
> 
> >  ##
> >  # @KvmInfo:
> > @@ -301,7 +302,7 @@
> >  # <- { "return": { "UUID": "550e8400-e29b-41d4-a716-446655440000" } }
> >  #
> >  ##
> > -{ 'command': 'query-uuid', 'returns': 'UuidInfo' }
> > +{ 'command': 'query-uuid', 'returns': 'UuidInfo', 'allow-preconfig': true }
> 
> Okay because qemu_uuid(optarg, &qemu_uuid) runs before -preconfig's
> main_loop().
> 
> >  
> >  ##
> >  # @EventInfo:
> > @@ -710,7 +711,8 @@
> >  #    }
> >  #
> >  ##
> > -{ 'command': 'query-iothreads', 'returns': ['IOThreadInfo'] }
> > +{ 'command': 'query-iothreads', 'returns': ['IOThreadInfo'],
> > +  'allow-preconfig': true }
> 
> Feeling lazy... why is this one okay?

Because it's a walk of the entire object tree; with
object_get_objects_root() - assuming the object tree exists we're safe.

> >  
> >  ##
> >  # @BalloonInfo:
> > @@ -2902,7 +2904,7 @@
> >  #    }
> >  #
> >  ##
> > -{ 'command': 'query-memdev', 'returns': ['Memdev'] }
> > +{ 'command': 'query-memdev', 'returns': ['Memdev'], 'allow-preconfig': true }
> >  
> >  ##
> >  # @PCDIMMDeviceInfo:
> 
> And this one?

Same trick as query-iothreads.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 5/7] hmp: Add info commands for preconfig

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Allow a bunch of the info commands to be used in preconfig.
> >
> > version, chardev, name, uuid,memdev, iothreads
> >   Were enabled in QMP in the previous patch from Igor
> 
> Yes, these are okay together with PATCH 4.
> 
> > status, hotpluggable_cpus
> >   Was enabled in the original allow-preconfig series
> 
> query-status looks okay to me.
> 
> > history
> >   is HMP specific
> 
> Yes.
> 
> > usbhost, qom-tree, numa
> >   Don't have a QMP equivalent
> 
> HMP commands without a QMP equivalent are okay if their functionality
> makes no sense in QMP, or is of use only for human users.
> 
> Example for "makes no sense in QMP": setting the current CPU, because a
> QMP monitor doesn't have a current CPU.
> 
> Examples for "is of use only for human users": HMP command "help", the
> integrated pocket calculator.

Right, but they do already exist; it's possible we may want to fix/add
QMP versions - but this series isn't about going through and fixing
existing stuff up.

> Now let's review the three commands:
> 
> * Gerd, why does "info usbhost" have no QMP equivalent?
> 
> * Eduardo, why does "info numa" have no QMP equivalent?
> 
> * "info qom-tree" is a recursive variant of qom-list that skips anything
>   but children.  This convenience command exists so you don't have to
>   filter and string together output from many qom-list.
> 
>   I think it stands to reason that if providing "info qom-tree" makes
>   sense, then so does qom-list (HMP and QMP).  If qom-list, then
>   qom-list-types, qom-list-properties, qom-get, and probably even
>   qom-set (I've always been suspicious of qom-set, but that has nothing
>   to do with preconfig state).
> 
>   It might make sense to split off the whole QOM shebang into a separate
>   patch.

People have been trying to add qom-get etc for quite a while (I tried a
couple of years ago); it gets stuck in type display issues.  I've not
directly seen a need for those other variants, but qom-get is something
I'd love to have, still that's a job for another patch.

'info qom-tree' is very very useful when debugging qemu to see what the
basic state we're building is; it's primarily for debugging.

Dave

> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 6/7] hmp: add exit_preconfig

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Add the exit_preconfig command to return to normality.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hmp-commands.hx | 15 +++++++++++++++
> >  hmp.c           |  7 +++++++
> >  hmp.h           |  1 +
> >  3 files changed, 23 insertions(+)
> >
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index dc82ed526f..40e2f6ca08 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -55,6 +55,21 @@ STEXI
> >  @item q or quit
> >  @findex quit
> >  Quit the emulator.
> > +ETEXI
> > +
> > +    {
> > +        .name       = "exit_preconfig",
> > +        .args_type  = "",
> > +        .params     = "",
> > +        .help       = "exit the preconfig state",
> > +        .cmd        = hmp_exit_preconfig,
> > +        .flags      = "p",
> > +    },
> > +
> > +STEXI
> > +@item exit_preconfig
> > +@findex exit_preconfig
> > +Exit the preconfig state
> 
> This is awfully terse.  Suggest to steal from the QMP documentation in
> misc.json:
> 
> # This command makes QEMU exit the preconfig state and proceed with
> # VM initialization using configuration data provided on the command line
> # and via the QMP monitor during the preconfig state. The command is only
> # available during the preconfig state (i.e. when the --preconfig command
> # line option was in use).

Changed.

> >  ETEXI
> >  
> >      {
> > diff --git a/hmp.c b/hmp.c
> > index ef93f4878b..c7be6ed394 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1065,6 +1065,13 @@ void hmp_system_powerdown(Monitor *mon, const QDict *qdict)
> >      qmp_system_powerdown(NULL);
> >  }
> >  
> > +void hmp_exit_preconfig(Monitor *mon, const QDict *qdict)
> > +{
> > +    Error *err = NULL;
> > +    qmp_exit_preconfig(&err);
> > +    hmp_handle_error(mon, &err);
> > +}
> > +
> 
> Blank line between declaration and statements, please.

Added.

Dave

> >  void hmp_cpu(Monitor *mon, const QDict *qdict)
> >  {
> >      int64_t cpu_index;
> > diff --git a/hmp.h b/hmp.h
> > index 20f27439d3..33354f1bdd 100644
> > --- a/hmp.h
> > +++ b/hmp.h
> > @@ -44,6 +44,7 @@ void hmp_quit(Monitor *mon, const QDict *qdict);
> >  void hmp_stop(Monitor *mon, const QDict *qdict);
> >  void hmp_system_reset(Monitor *mon, const QDict *qdict);
> >  void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
> > +void hmp_exit_preconfig(Monitor *mon, const QDict *qdict);
> >  void hmp_cpu(Monitor *mon, const QDict *qdict);
> >  void hmp_memsave(Monitor *mon, const QDict *qdict);
> >  void hmp_pmemsave(Monitor *mon, const QDict *qdict);
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 5/7] hmp: Add info commands for preconfig

[Eduardo Habkost]

On Mon, Jun 11, 2018 at 02:01:52PM +0200, Markus Armbruster wrote:
> * Eduardo, why does "info numa" have no QMP equivalent?

Nobody ever asked for one, which seems to qualify as "only for
human users".

Should we add an equivalent QMP command even if we don't expect
anybody to use it?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 2/7] hmp: Allow help on preconfig commands

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> >> 
> >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >> >
> >> > Allow the 'help' command in preconfig state but
> >> > make it only list the preconfig commands.
> >> >
> >> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> > Reviewed-by: Peter Xu <peterx@redhat.com>
> >> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> >> > ---
> >> >  hmp-commands.hx | 1 +
> >> >  monitor.c       | 8 +++++++-
> >> >  2 files changed, 8 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> >> > index 0734fea931..8bf590ae4b 100644
> >> > --- a/hmp-commands.hx
> >> > +++ b/hmp-commands.hx
> >> > @@ -15,6 +15,7 @@ ETEXI
> >> >          .params     = "[cmd]",
> >> >          .help       = "show the help",
> >> >          .cmd        = do_help_cmd,
> >> > +        .flags      = "p",
> >> >      },
> >> >  
> >> >  STEXI
> >> > diff --git a/monitor.c b/monitor.c
> >> > index f4a16e6a03..31c8f5dc88 100644
> >> > --- a/monitor.c
> >> > +++ b/monitor.c
> >> > @@ -957,6 +957,10 @@ static void help_cmd_dump_one(Monitor *mon,
> >> >  {
> >> >      int i;
> >> >  
> >> > +    if (runstate_check(RUN_STATE_PRECONFIG) && !cmd_can_preconfig(cmd)) {
> >> > +        return;
> >> > +    }
> >> > +
> >> >      for (i = 0; i < prefix_args_nb; i++) {
> >> >          monitor_printf(mon, "%s ", prefix_args[i]);
> >> >      }
> >> > @@ -979,7 +983,9 @@ static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
> >> >  
> >> >      /* Find one entry to dump */
> >> >      for (cmd = cmds; cmd->name != NULL; cmd++) {
> >> > -        if (compare_cmd(args[arg_index], cmd->name)) {
> >> > +        if (compare_cmd(args[arg_index], cmd->name) &&
> >> > +            ((!runstate_check(RUN_STATE_PRECONFIG) ||
> >> > +                cmd_can_preconfig(cmd)))) {
> >> 
> >> Why do we need this check in addition to the one in help_cmd_dump_one()?
> >
> > To gate entire subtables (we've only currently got 'info' and that's enabled,
> > anyway, so it never actually triggers).
> 
> Something's bothering me around here, but I can't put a finger on it...
> let me think aloud.
> 
> There's an asymmetry between command execution and help.  The former has
> just one check, in monitor_parse_command().  If the command has a
> sub-table, and there are arguments, monitor_parse_command() recurses
> into the sub-table.  Thus, if the command is unavailable, the
> sub-commands are unavailable as well, regardless of their "p" flags.
> 
> Help's recursion is different.  It's limited to two levels, unlike
> execution.  help_cmd_dump_one()'s check applies to the last level.
> help_cmd_dump()'s check applies to the first level.  If there's just one
> level, we check twice.  Perhaps less than elegant, but harmless and
> simple.
> 
> You can't make a sub-command available without also making the command
> available.  Makes sense, I guess.  If you try, your "p" flags on the
> sub-commands are silently ignored.  That's a bit ugly, but if it doesn't
> bother you, I can pretend I didn't see it ;)

Yes I'm OK with that; you've got to enable all the way down to the
command that youw ant to enable.

I think the difference between execution and help comes from the
way you can do 'help' on it's own to list everything, and help
on a subtable (help info) and help on a command (help info uuid);
I think that's why the recursion gets a bit hairier.

Dave

> > Dave
> >
> >> >              if (cmd->sub_table) {
> >> >                  /* continue with next arg */
> >> >                  help_cmd_dump(mon, cmd->sub_table,
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 5/7] hmp: Add info commands for preconfig

[Igor Mammedov]

On Mon, 11 Jun 2018 15:40:16 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Jun 11, 2018 at 02:01:52PM +0200, Markus Armbruster wrote:
> > * Eduardo, why does "info numa" have no QMP equivalent?
> 
> Nobody ever asked for one, which seems to qualify as "only for
> human users".
> 
> Should we add an equivalent QMP command even if we don't expect
> anybody to use it?

we inderectly can fetch numa info via QMP, using 
  query-hotpluggable-cpus
for CPU mapping and
  query-memory-devices
for (NV|PC)-dimm devices, however there is no QMP way for getting
for numa mapping of initial RAM nor configured numa nodes
(not counting querying CLI options).

So perhaps we need info 'numa' equivalent for QMP which would give
the same amount of information as HMP in one query.

Maybe libvirt side as actual users know better if it's really needed (CCed)

Re: [Qemu-devel] [PATCH v3 5/7] hmp: Add info commands for preconfig

[Gerd Hoffmann]

  Hi,

> > Now let's review the three commands:
> > 
> > * Gerd, why does "info usbhost" have no QMP equivalent?

Works only when running qemu directly, in the libvirt sandbox qemu
hasn't the permissions needed to scan the host usb bus so that would be
rather pointless ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v3 5/7] hmp: Add info commands for preconfig

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
>> 
>> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> >
>> > Allow a bunch of the info commands to be used in preconfig.
>> >
>> > version, chardev, name, uuid,memdev, iothreads
>> >   Were enabled in QMP in the previous patch from Igor
>> 
>> Yes, these are okay together with PATCH 4.
>> 
>> > status, hotpluggable_cpus
>> >   Was enabled in the original allow-preconfig series
>> 
>> query-status looks okay to me.
>> 
>> > history
>> >   is HMP specific
>> 
>> Yes.
>> 
>> > usbhost, qom-tree, numa
>> >   Don't have a QMP equivalent
>> 
>> HMP commands without a QMP equivalent are okay if their functionality
>> makes no sense in QMP, or is of use only for human users.
>> 
>> Example for "makes no sense in QMP": setting the current CPU, because a
>> QMP monitor doesn't have a current CPU.
>> 
>> Examples for "is of use only for human users": HMP command "help", the
>> integrated pocket calculator.
>
> Right, but they do already exist; it's possible we may want to fix/add
> QMP versions - but this series isn't about going through and fixing
> existing stuff up.
>
>> Now let's review the three commands:
>> 
>> * Gerd, why does "info usbhost" have no QMP equivalent?
>> 
>> * Eduardo, why does "info numa" have no QMP equivalent?
>> 
>> * "info qom-tree" is a recursive variant of qom-list that skips anything
>>   but children.  This convenience command exists so you don't have to
>>   filter and string together output from many qom-list.
>> 
>>   I think it stands to reason that if providing "info qom-tree" makes
>>   sense, then so does qom-list (HMP and QMP).  If qom-list, then
>>   qom-list-types, qom-list-properties, qom-get, and probably even
>>   qom-set (I've always been suspicious of qom-set, but that has nothing
>>   to do with preconfig state).
>> 
>>   It might make sense to split off the whole QOM shebang into a separate
>>   patch.
>
> People have been trying to add qom-get etc for quite a while (I tried a
> couple of years ago); it gets stuck in type display issues.  I've not
> directly seen a need for those other variants, but qom-get is something
> I'd love to have, still that's a job for another patch.

Yes.

> 'info qom-tree' is very very useful when debugging qemu to see what the
> basic state we're building is; it's primarily for debugging.

I'm not at all opposed to enabling qom-tree, but I want its QMP building
blocks enabled as well then.  I think enabling their HMP buddies as well
would only make sense.

Re: [Qemu-devel] [PATCH v3 5/7] hmp: Add info commands for preconfig

[Markus Armbruster]

Igor Mammedov <imammedo@redhat.com> writes:

> On Mon, 11 Jun 2018 15:40:16 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
>> On Mon, Jun 11, 2018 at 02:01:52PM +0200, Markus Armbruster wrote:
>> > * Eduardo, why does "info numa" have no QMP equivalent?
>> 
>> Nobody ever asked for one, which seems to qualify as "only for
>> human users".
>> 
>> Should we add an equivalent QMP command even if we don't expect
>> anybody to use it?

The hard requirement for QMP from day one was "provide everything
machine clients need".  To avoid speculation and endless arguments about
what might be needed / not needed, we resolved to approximate this by
"provide everything, except stuff that's *clearly* of no use to
machines".

When you think a command is such an exception, you should explain why in
its commit message.

Note that HMP need not provide the functionality in the exact same
packaging.  For instance, it's fine to have building blocks in QMP, and
just a high-level command in HMP.  However, the latter must be
implemented with the building blocks to make it obvious that QMP
provides the same functionality.

For additional references, see
Message-ID: <87d0x1p6go.fsf@dusky.pond.sub.org>
https://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg02176.html

> we inderectly can fetch numa info via QMP, using 
>   query-hotpluggable-cpus
> for CPU mapping and
>   query-memory-devices
> for (NV|PC)-dimm devices, however there is no QMP way for getting
> for numa mapping of initial RAM nor configured numa nodes
> (not counting querying CLI options).

Sounds like most of the building blocks are already there.  The
"obviousness" isn't.

> So perhaps we need info 'numa' equivalent for QMP which would give
> the same amount of information as HMP in one query.

I'd appreciate patches to get us to "QMP has the building blocks, and
HMP is implemented with them".

> Maybe libvirt side as actual users know better if it's really needed (CCed)

Always welcome.

Re: [Qemu-devel] [PATCH v3 2/7] hmp: Allow help on preconfig commands

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>> 
>> > * Markus Armbruster (armbru@redhat.com) wrote:
>> >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
>> >> 
>> >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> >> >
>> >> > Allow the 'help' command in preconfig state but
>> >> > make it only list the preconfig commands.
>> >> >
>> >> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> >> > Reviewed-by: Peter Xu <peterx@redhat.com>
>> >> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> >> > ---
>> >> >  hmp-commands.hx | 1 +
>> >> >  monitor.c       | 8 +++++++-
>> >> >  2 files changed, 8 insertions(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/hmp-commands.hx b/hmp-commands.hx
>> >> > index 0734fea931..8bf590ae4b 100644
>> >> > --- a/hmp-commands.hx
>> >> > +++ b/hmp-commands.hx
>> >> > @@ -15,6 +15,7 @@ ETEXI
>> >> >          .params     = "[cmd]",
>> >> >          .help       = "show the help",
>> >> >          .cmd        = do_help_cmd,
>> >> > +        .flags      = "p",
>> >> >      },
>> >> >  
>> >> >  STEXI
>> >> > diff --git a/monitor.c b/monitor.c
>> >> > index f4a16e6a03..31c8f5dc88 100644
>> >> > --- a/monitor.c
>> >> > +++ b/monitor.c
>> >> > @@ -957,6 +957,10 @@ static void help_cmd_dump_one(Monitor *mon,
>> >> >  {
>> >> >      int i;
>> >> >  
>> >> > +    if (runstate_check(RUN_STATE_PRECONFIG) && !cmd_can_preconfig(cmd)) {
>> >> > +        return;
>> >> > +    }
>> >> > +
>> >> >      for (i = 0; i < prefix_args_nb; i++) {
>> >> >          monitor_printf(mon, "%s ", prefix_args[i]);
>> >> >      }
>> >> > @@ -979,7 +983,9 @@ static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
>> >> >  
>> >> >      /* Find one entry to dump */
>> >> >      for (cmd = cmds; cmd->name != NULL; cmd++) {
>> >> > -        if (compare_cmd(args[arg_index], cmd->name)) {
>> >> > +        if (compare_cmd(args[arg_index], cmd->name) &&
>> >> > +            ((!runstate_check(RUN_STATE_PRECONFIG) ||
>> >> > +                cmd_can_preconfig(cmd)))) {
>> >> 
>> >> Why do we need this check in addition to the one in help_cmd_dump_one()?
>> >
>> > To gate entire subtables (we've only currently got 'info' and that's enabled,
>> > anyway, so it never actually triggers).
>> 
>> Something's bothering me around here, but I can't put a finger on it...
>> let me think aloud.
>> 
>> There's an asymmetry between command execution and help.  The former has
>> just one check, in monitor_parse_command().  If the command has a
>> sub-table, and there are arguments, monitor_parse_command() recurses
>> into the sub-table.  Thus, if the command is unavailable, the
>> sub-commands are unavailable as well, regardless of their "p" flags.
>> 
>> Help's recursion is different.  It's limited to two levels, unlike
>> execution.  help_cmd_dump_one()'s check applies to the last level.
>> help_cmd_dump()'s check applies to the first level.  If there's just one
>> level, we check twice.  Perhaps less than elegant, but harmless and
>> simple.
>> 
>> You can't make a sub-command available without also making the command
>> available.  Makes sense, I guess.  If you try, your "p" flags on the
>> sub-commands are silently ignored.  That's a bit ugly, but if it doesn't
>> bother you, I can pretend I didn't see it ;)
>
> Yes I'm OK with that; you've got to enable all the way down to the
> command that youw ant to enable.
>
> I think the difference between execution and help comes from the
> way you can do 'help' on it's own to list everything, and help
> on a subtable (help info) and help on a command (help info uuid);
> I think that's why the recursion gets a bit hairier.

Well, you can execute the command on its own (info), and a sub-command
(info uuid) --- same structure.  Anyway, what we have works.

Re: [Qemu-devel] [PATCH v3 4/7] qmp: enable query-[chardev|version|name|uuid|iothreads|memdev] commands in preconfig state

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
>> 
>> > From: Igor Mammedov <imammedo@redhat.com>
>> >
>> > subj commands, are informational and do not depend on machine being
>> > initialized. Make them available early in preconfig runstate to make
>> > the later a little bit more useful.
>> >
>> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> 
>> Excessively long subject line.  Suggested fix:
>> 
>>     qmp: Enable a few query- commands in preconfig state
>> 
>>     Commands query-chardev, query-version, query-name, query-uuid,
>>     query-iothreads, query-memdev are informational and do not depend on
>>     the machine being initialized.  Make them available in preconfig
>>     runstate to make the latter a little bit more useful.
>> 
>> > ---
>> >  qapi/char.json |  3 ++-
>> >  qapi/misc.json | 12 +++++++-----
>> >  2 files changed, 9 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/qapi/char.json b/qapi/char.json
>> > index ae19dcd1ed..60f31d83fc 100644
>> > --- a/qapi/char.json
>> > +++ b/qapi/char.json
>> > @@ -62,7 +62,8 @@
>> >  #    }
>> >  #
>> >  ##
>> > -{ 'command': 'query-chardev', 'returns': ['ChardevInfo'] }
>> > +{ 'command': 'query-chardev', 'returns': ['ChardevInfo'],
>> > +  'allow-preconfig': true }
>> 
>> Okay because
>> 
>>     if (qemu_opts_foreach(qemu_find_opts("chardev"),
>>                           chardev_init_func, NULL, NULL)) {
>>         exit(1);
>>     }
>> 
>> runs before -preconfig's main_loop().
>> 
>> >  
>> >  ##
>> >  # @ChardevBackendInfo:
>> > diff --git a/qapi/misc.json b/qapi/misc.json
>> > index f83a63a0ab..1be1728c0e 100644
>> > --- a/qapi/misc.json
>> > +++ b/qapi/misc.json
>> > @@ -117,7 +117,8 @@
>> >  #    }
>> >  #
>> >  ##
>> > -{ 'command': 'query-version', 'returns': 'VersionInfo' }
>> > +{ 'command': 'query-version', 'returns': 'VersionInfo',
>> > +  'allow-preconfig': true }
>> 
>> Returns data fixed at compile time.  Okay.
>> 
>> >  
>> >  ##
>> >  # @CommandInfo:
>> > @@ -241,7 +242,7 @@
>> >  # <- { "return": { "name": "qemu-name" } }
>> >  #
>> >  ##
>> > -{ 'command': 'query-name', 'returns': 'NameInfo' }
>> > +{ 'command': 'query-name', 'returns': 'NameInfo', 'allow-preconfig': true }
>> 
>> Okay because parse_name() runs before -preconfig's main_loop().
>> 
>> >  ##
>> >  # @KvmInfo:
>> > @@ -301,7 +302,7 @@
>> >  # <- { "return": { "UUID": "550e8400-e29b-41d4-a716-446655440000" } }
>> >  #
>> >  ##
>> > -{ 'command': 'query-uuid', 'returns': 'UuidInfo' }
>> > +{ 'command': 'query-uuid', 'returns': 'UuidInfo', 'allow-preconfig': true }
>> 
>> Okay because qemu_uuid(optarg, &qemu_uuid) runs before -preconfig's
>> main_loop().
>> 
>> >  
>> >  ##
>> >  # @EventInfo:
>> > @@ -710,7 +711,8 @@
>> >  #    }
>> >  #
>> >  ##
>> > -{ 'command': 'query-iothreads', 'returns': ['IOThreadInfo'] }
>> > +{ 'command': 'query-iothreads', 'returns': ['IOThreadInfo'],
>> > +  'allow-preconfig': true }
>> 
>> Feeling lazy... why is this one okay?
>
> Because it's a walk of the entire object tree; with
> object_get_objects_root() - assuming the object tree exists we're safe.
>
>> >  
>> >  ##
>> >  # @BalloonInfo:
>> > @@ -2902,7 +2904,7 @@
>> >  #    }
>> >  #
>> >  ##
>> > -{ 'command': 'query-memdev', 'returns': ['Memdev'] }
>> > +{ 'command': 'query-memdev', 'returns': ['Memdev'], 'allow-preconfig': true }
>> >  
>> >  ##
>> >  # @PCDIMMDeviceInfo:
>> 
>> And this one?
>
> Same trick as query-iothreads.

Thanks.

Re: [Qemu-devel] [PATCH v3 5/7] hmp: Add info commands for preconfig

[Daniel P. Berrangé]

On Mon, Jun 11, 2018 at 11:33:42PM +0200, Igor Mammedov wrote:
> On Mon, 11 Jun 2018 15:40:16 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Jun 11, 2018 at 02:01:52PM +0200, Markus Armbruster wrote:
> > > * Eduardo, why does "info numa" have no QMP equivalent?
> > 
> > Nobody ever asked for one, which seems to qualify as "only for
> > human users".
> > 
> > Should we add an equivalent QMP command even if we don't expect
> > anybody to use it?
> 
> we inderectly can fetch numa info via QMP, using 
>   query-hotpluggable-cpus
> for CPU mapping and
>   query-memory-devices
> for (NV|PC)-dimm devices, however there is no QMP way for getting
> for numa mapping of initial RAM nor configured numa nodes
> (not counting querying CLI options).
> 
> So perhaps we need info 'numa' equivalent for QMP which would give
> the same amount of information as HMP in one query.
> 
> Maybe libvirt side as actual users know better if it's really needed (CCed)

Libvirt knows the numa config it used to spawn the guest with, so there's
no immediate compelling reason to query it back, as QEMU doesn't change
it later.

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

Re: [Qemu-devel] [PATCH v3 5/7] hmp: Add info commands for preconfig

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> >> 
> >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >> >
> >> > Allow a bunch of the info commands to be used in preconfig.
> >> >
> >> > version, chardev, name, uuid,memdev, iothreads
> >> >   Were enabled in QMP in the previous patch from Igor
> >> 
> >> Yes, these are okay together with PATCH 4.
> >> 
> >> > status, hotpluggable_cpus
> >> >   Was enabled in the original allow-preconfig series
> >> 
> >> query-status looks okay to me.
> >> 
> >> > history
> >> >   is HMP specific
> >> 
> >> Yes.
> >> 
> >> > usbhost, qom-tree, numa
> >> >   Don't have a QMP equivalent
> >> 
> >> HMP commands without a QMP equivalent are okay if their functionality
> >> makes no sense in QMP, or is of use only for human users.
> >> 
> >> Example for "makes no sense in QMP": setting the current CPU, because a
> >> QMP monitor doesn't have a current CPU.
> >> 
> >> Examples for "is of use only for human users": HMP command "help", the
> >> integrated pocket calculator.
> >
> > Right, but they do already exist; it's possible we may want to fix/add
> > QMP versions - but this series isn't about going through and fixing
> > existing stuff up.
> >
> >> Now let's review the three commands:
> >> 
> >> * Gerd, why does "info usbhost" have no QMP equivalent?
> >> 
> >> * Eduardo, why does "info numa" have no QMP equivalent?
> >> 
> >> * "info qom-tree" is a recursive variant of qom-list that skips anything
> >>   but children.  This convenience command exists so you don't have to
> >>   filter and string together output from many qom-list.
> >> 
> >>   I think it stands to reason that if providing "info qom-tree" makes
> >>   sense, then so does qom-list (HMP and QMP).  If qom-list, then
> >>   qom-list-types, qom-list-properties, qom-get, and probably even
> >>   qom-set (I've always been suspicious of qom-set, but that has nothing
> >>   to do with preconfig state).
> >> 
> >>   It might make sense to split off the whole QOM shebang into a separate
> >>   patch.
> >
> > People have been trying to add qom-get etc for quite a while (I tried a
> > couple of years ago); it gets stuck in type display issues.  I've not
> > directly seen a need for those other variants, but qom-get is something
> > I'd love to have, still that's a job for another patch.
> 
> Yes.
> 
> > 'info qom-tree' is very very useful when debugging qemu to see what the
> > basic state we're building is; it's primarily for debugging.
> 
> I'm not at all opposed to enabling qom-tree, but I want its QMP building
> blocks enabled as well then.  I think enabling their HMP buddies as well
> would only make sense.

Hmm; OK, enabling qom-list, qom-get, qom-set, qom-list-types,
qom-list-properties for qmp.
qom-list and qom-set enabled in HMP.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 5/7] hmp: Add info commands for preconfig

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> > Now let's review the three commands:
>> > 
>> > * Gerd, why does "info usbhost" have no QMP equivalent?
>
> Works only when running qemu directly, in the libvirt sandbox qemu
> hasn't the permissions needed to scan the host usb bus so that would be
> rather pointless ...

I don't think this meets either of the two criteria:

* I meets "makes no sense in QMP" only if QMP implies "can't scan host
  USB bus".  Libvirt implies it, but QMP doesn't imply libvirt; it's its
  most important, not its sole user.

* It meets "of use only for human users" only if we're convinced it's of
  no use to programs.

  To avoid speculation and endless arguments about what could or could
  not be of use, we've always stuck to "when in doubt, assume it could
  be of use".

  "Libvirt can't use it" falls short.

  "Any management application worth anything would deny QEMU the
  capability to scan the USB host bus, and thus wouldn't be able to use
  it" is exactly the argument we intended to avoid.

QMP falling short of completeness in relatively unimportant ways like
this one isn't exactly terrible.  The most serious effect is probably
serving as a bad example that leads to further arguments like this one.
These are well worth avoiding, though.

Re: [Qemu-devel] [PATCH v3 5/7] hmp: Add info commands for preconfig

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Gerd Hoffmann <kraxel@redhat.com> writes:
> 
> >   Hi,
> >
> >> > Now let's review the three commands:
> >> > 
> >> > * Gerd, why does "info usbhost" have no QMP equivalent?
> >
> > Works only when running qemu directly, in the libvirt sandbox qemu
> > hasn't the permissions needed to scan the host usb bus so that would be
> > rather pointless ...
> 
> I don't think this meets either of the two criteria:
> 
> * I meets "makes no sense in QMP" only if QMP implies "can't scan host
>   USB bus".  Libvirt implies it, but QMP doesn't imply libvirt; it's its
>   most important, not its sole user.
> 
> * It meets "of use only for human users" only if we're convinced it's of
>   no use to programs.
> 
>   To avoid speculation and endless arguments about what could or could
>   not be of use, we've always stuck to "when in doubt, assume it could
>   be of use".
> 
>   "Libvirt can't use it" falls short.
> 
>   "Any management application worth anything would deny QEMU the
>   capability to scan the USB host bus, and thus wouldn't be able to use
>   it" is exactly the argument we intended to avoid.
> 
> QMP falling short of completeness in relatively unimportant ways like
> this one isn't exactly terrible.  The most serious effect is probably
> serving as a bad example that leads to further arguments like this one.
> These are well worth avoiding, though.

Markus:
  a) This is a separate discussion; info usbhost has been there for many
years;  this patch set doesn't change that.
  b) From HMP, if someone wants to add a command like 'info usbhost' to
make their debugging of USB easy, then HMP is all for that and there's
no way I'm going to require QMP implementations for a debug command.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 5/7] hmp: Add info commands for preconfig

[Eduardo Habkost]

On Tue, Jun 12, 2018 at 09:00:20AM +0200, Markus Armbruster wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > On Mon, 11 Jun 2018 15:40:16 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> >> On Mon, Jun 11, 2018 at 02:01:52PM +0200, Markus Armbruster wrote:
> >> > * Eduardo, why does "info numa" have no QMP equivalent?
> >> 
> >> Nobody ever asked for one, which seems to qualify as "only for
> >> human users".
> >> 
> >> Should we add an equivalent QMP command even if we don't expect
> >> anybody to use it?
> 
> The hard requirement for QMP from day one was "provide everything
> machine clients need".  To avoid speculation and endless arguments about
> what might be needed / not needed, we resolved to approximate this by
> "provide everything, except stuff that's *clearly* of no use to
> machines".
> 
> When you think a command is such an exception, you should explain why in
> its commit message.

It's not an exception nor it needs to be one.  I was just not
aware of the above approximation.


> 
> Note that HMP need not provide the functionality in the exact same
> packaging.  For instance, it's fine to have building blocks in QMP, and
> just a high-level command in HMP.  However, the latter must be
> implemented with the building blocks to make it obvious that QMP
> provides the same functionality.
> 
> For additional references, see
> Message-ID: <87d0x1p6go.fsf@dusky.pond.sub.org>
> https://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg02176.html
> 
> > we inderectly can fetch numa info via QMP, using 
> >   query-hotpluggable-cpus
> > for CPU mapping and
> >   query-memory-devices
> > for (NV|PC)-dimm devices, however there is no QMP way for getting
> > for numa mapping of initial RAM nor configured numa nodes
> > (not counting querying CLI options).
> 
> Sounds like most of the building blocks are already there.  The
> "obviousness" isn't.
> 
> > So perhaps we need info 'numa' equivalent for QMP which would give
> > the same amount of information as HMP in one query.
> 
> I'd appreciate patches to get us to "QMP has the building blocks, and
> HMP is implemented with them".

Agreed.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 5/7] hmp: Add info commands for preconfig

[Eduardo Habkost]

On Tue, Jun 12, 2018 at 09:49:14AM +0100, Dr. David Alan Gilbert wrote:
[...]
> > > People have been trying to add qom-get etc for quite a while (I tried a
> > > couple of years ago); it gets stuck in type display issues.  I've not
> > > directly seen a need for those other variants, but qom-get is something
> > > I'd love to have, still that's a job for another patch.
> > 
> > Yes.
> > 
> > > 'info qom-tree' is very very useful when debugging qemu to see what the
> > > basic state we're building is; it's primarily for debugging.
> > 
> > I'm not at all opposed to enabling qom-tree, but I want its QMP building
> > blocks enabled as well then.  I think enabling their HMP buddies as well
> > would only make sense.
> 
> Hmm; OK, enabling qom-list, qom-get, qom-set, qom-list-types,
> qom-list-properties for qmp.
> qom-list and qom-set enabled in HMP.

I would prefer to avoid enabling qom-set in preconfig mode unless
we have a good reason for that.  I don't trust all existing
property setters to not crash or break if the machine is not
initialized yet.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 5/7] hmp: Add info commands for preconfig

[Daniel P. Berrangé]

On Wed, Jun 13, 2018 at 10:47:45AM -0300, Eduardo Habkost wrote:
> On Tue, Jun 12, 2018 at 09:49:14AM +0100, Dr. David Alan Gilbert wrote:
> [...]
> > > > People have been trying to add qom-get etc for quite a while (I tried a
> > > > couple of years ago); it gets stuck in type display issues.  I've not
> > > > directly seen a need for those other variants, but qom-get is something
> > > > I'd love to have, still that's a job for another patch.
> > > 
> > > Yes.
> > > 
> > > > 'info qom-tree' is very very useful when debugging qemu to see what the
> > > > basic state we're building is; it's primarily for debugging.
> > > 
> > > I'm not at all opposed to enabling qom-tree, but I want its QMP building
> > > blocks enabled as well then.  I think enabling their HMP buddies as well
> > > would only make sense.
> > 
> > Hmm; OK, enabling qom-list, qom-get, qom-set, qom-list-types,
> > qom-list-properties for qmp.
> > qom-list and qom-set enabled in HMP.
> 
> I would prefer to avoid enabling qom-set in preconfig mode unless
> we have a good reason for that.  I don't trust all existing
> property setters to not crash or break if the machine is not
> initialized yet.

If we're in early startup, the impact of a crash is pretty minor - QEMU
will simply exit, which is not significantly differnt from if a setter
handled it "correctly" by setting an Error **errp. A mgmt app that uses
this and finds a setter which crashes will detect the problem pretty
quickly & report bugs.

QEMU is complex enough that we're unlikely to ever find broken setters
by code inspection. So if we don't allow it in preconfig mode, then
I doubt we'll ever find & fix the problems.

So, IMHO, we should allow qom-set and just fix problems as they come
to light, as we would for any other part of QEMU which has plenty of
scope for crashers in rarely tested codepaths.

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

Re: [Qemu-devel] [PATCH v3 5/7] hmp: Add info commands for preconfig

[Eduardo Habkost]

On Wed, Jun 13, 2018 at 02:53:37PM +0100, Daniel P. Berrangé wrote:
> On Wed, Jun 13, 2018 at 10:47:45AM -0300, Eduardo Habkost wrote:
> > On Tue, Jun 12, 2018 at 09:49:14AM +0100, Dr. David Alan Gilbert wrote:
> > [...]
> > > > > People have been trying to add qom-get etc for quite a while (I tried a
> > > > > couple of years ago); it gets stuck in type display issues.  I've not
> > > > > directly seen a need for those other variants, but qom-get is something
> > > > > I'd love to have, still that's a job for another patch.
> > > > 
> > > > Yes.
> > > > 
> > > > > 'info qom-tree' is very very useful when debugging qemu to see what the
> > > > > basic state we're building is; it's primarily for debugging.
> > > > 
> > > > I'm not at all opposed to enabling qom-tree, but I want its QMP building
> > > > blocks enabled as well then.  I think enabling their HMP buddies as well
> > > > would only make sense.
> > > 
> > > Hmm; OK, enabling qom-list, qom-get, qom-set, qom-list-types,
> > > qom-list-properties for qmp.
> > > qom-list and qom-set enabled in HMP.
> > 
> > I would prefer to avoid enabling qom-set in preconfig mode unless
> > we have a good reason for that.  I don't trust all existing
> > property setters to not crash or break if the machine is not
> > initialized yet.
> 
> If we're in early startup, the impact of a crash is pretty minor - QEMU
> will simply exit, which is not significantly differnt from if a setter
> handled it "correctly" by setting an Error **errp. A mgmt app that uses
> this and finds a setter which crashes will detect the problem pretty
> quickly & report bugs.
> 
> QEMU is complex enough that we're unlikely to ever find broken setters
> by code inspection. So if we don't allow it in preconfig mode, then
> I doubt we'll ever find & fix the problems.
> 
> So, IMHO, we should allow qom-set and just fix problems as they come
> to light, as we would for any other part of QEMU which has plenty of
> scope for crashers in rarely tested codepaths.

Good points.

I'm still worried about making qom-set confused with a supported
configuration API, and have applications starting to rely on it.

But I guess this problem could be solved by properly documenting
qom-set as a debugging aid, not a supported API.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 0/7] Reenable hmp for preconfig mode

[Igor Mammedov]

On Mon, 11 Jun 2018 14:44:41 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:  
> >> Let's also provide "quit", both in HMP and QMP.  
> >
> > I tried enabling quit, but it fails with an incorrect
> > state transition.  
> 
> Igor, this one's for you.

I'll look into it

[Qemu-devel] Abandon our QMP first policy? (was: [PATCH v3 5/7] hmp: Add info commands for preconfig)

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Gerd Hoffmann <kraxel@redhat.com> writes:
>> 
>> >   Hi,
>> >
>> >> > Now let's review the three commands:
>> >> > 
>> >> > * Gerd, why does "info usbhost" have no QMP equivalent?
>> >
>> > Works only when running qemu directly, in the libvirt sandbox qemu
>> > hasn't the permissions needed to scan the host usb bus so that would be
>> > rather pointless ...
>> 
>> I don't think this meets either of the two criteria:
>> 
>> * I meets "makes no sense in QMP" only if QMP implies "can't scan host
>>   USB bus".  Libvirt implies it, but QMP doesn't imply libvirt; it's its
>>   most important, not its sole user.
>> 
>> * It meets "of use only for human users" only if we're convinced it's of
>>   no use to programs.
>> 
>>   To avoid speculation and endless arguments about what could or could
>>   not be of use, we've always stuck to "when in doubt, assume it could
>>   be of use".
>> 
>>   "Libvirt can't use it" falls short.
>> 
>>   "Any management application worth anything would deny QEMU the
>>   capability to scan the USB host bus, and thus wouldn't be able to use
>>   it" is exactly the argument we intended to avoid.
>> 
>> QMP falling short of completeness in relatively unimportant ways like
>> this one isn't exactly terrible.  The most serious effect is probably
>> serving as a bad example that leads to further arguments like this one.
>> These are well worth avoiding, though.
>
> Markus:
>   a) This is a separate discussion; info usbhost has been there for many
> years;  this patch set doesn't change that.

Yes, it's separate.

>   b) From HMP, if someone wants to add a command like 'info usbhost' to
> make their debugging of USB easy, then HMP is all for that and there's
> no way I'm going to require QMP implementations for a debug command.

As I said before, said someone is welcome to make his case for why the
command is of use only for human users in the commit message.

If the QMP maintainer gets a say in judging that case, good.

But if your "no way" means what I have to assume it means, namely "no
way I'm taking no for an answer", then we're at an impasse.

Evolving HMP without consideration for QMP's mission makes carrying out
that mission harder.  Specifically, me having to watch for new HMP
commands bypassing QMP, then judge and maybe debate whether that's okay
is beyond my capacity.  The debate part in particular.  I expect to fail
as a QMP maintainer in that case.

Avoiding QMP maintenance balloon out of control was one of our reasons
for the rule "HMP commands must be implemented in terms of QMP".  We pay
a modest amount of extra developer effort for a much-needed
simplification of the maintainers' job.

Regarding "modest amount of effort": I already told you I'm prepared to
back that claim with patches whenever requiring QMP blocks HMP patches.
If that can't shake your conviction that requiring QMP is impractical, I
figure nothing will.

Ways out of the impasse:

1. Find a more capable maintainer for QMP (MAINTAINERS sections QMP and
QAPI Schema).  To give you an idea of the work involved, almost 10% of
the commits in 2.9 touched the latter.

2. Split the work differently: make the HMP maintainer responsible for
keeping QMP complete.

3. Abandon QMP's goal to be complete.  Tell management applications to
use HMP where QMP falls short.

To be brutally frank, 2. requires more trust in your willingness to care
about QMP than I have right now.

By refusing to cooperate, you can force 3.

The solution most conducive to my personal well-being is probably 1.

Any other bright ideas?

Separate discussion, does not block this series.

Re: [Qemu-devel] Abandon our QMP first policy? (was: [PATCH v3 5/7] hmp: Add info commands for preconfig)

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> Gerd Hoffmann <kraxel@redhat.com> writes:
> >> 
> >> >   Hi,
> >> >
> >> >> > Now let's review the three commands:
> >> >> > 
> >> >> > * Gerd, why does "info usbhost" have no QMP equivalent?
> >> >
> >> > Works only when running qemu directly, in the libvirt sandbox qemu
> >> > hasn't the permissions needed to scan the host usb bus so that would be
> >> > rather pointless ...
> >> 
> >> I don't think this meets either of the two criteria:
> >> 
> >> * I meets "makes no sense in QMP" only if QMP implies "can't scan host
> >>   USB bus".  Libvirt implies it, but QMP doesn't imply libvirt; it's its
> >>   most important, not its sole user.
> >> 
> >> * It meets "of use only for human users" only if we're convinced it's of
> >>   no use to programs.
> >> 
> >>   To avoid speculation and endless arguments about what could or could
> >>   not be of use, we've always stuck to "when in doubt, assume it could
> >>   be of use".
> >> 
> >>   "Libvirt can't use it" falls short.
> >> 
> >>   "Any management application worth anything would deny QEMU the
> >>   capability to scan the USB host bus, and thus wouldn't be able to use
> >>   it" is exactly the argument we intended to avoid.
> >> 
> >> QMP falling short of completeness in relatively unimportant ways like
> >> this one isn't exactly terrible.  The most serious effect is probably
> >> serving as a bad example that leads to further arguments like this one.
> >> These are well worth avoiding, though.
> >
> > Markus:
> >   a) This is a separate discussion; info usbhost has been there for many
> > years;  this patch set doesn't change that.
> 
> Yes, it's separate.
> 
> >   b) From HMP, if someone wants to add a command like 'info usbhost' to
> > make their debugging of USB easy, then HMP is all for that and there's
> > no way I'm going to require QMP implementations for a debug command.
> 
> As I said before, said someone is welcome to make his case for why the
> command is of use only for human users in the commit message.
> 
> If the QMP maintainer gets a say in judging that case, good.
> 
> But if your "no way" means what I have to assume it means, namely "no
> way I'm taking no for an answer", then we're at an impasse.
> 
> Evolving HMP without consideration for QMP's mission makes carrying out
> that mission harder.  Specifically, me having to watch for new HMP
> commands bypassing QMP, then judge and maybe debate whether that's okay
> is beyond my capacity.  The debate part in particular.  I expect to fail
> as a QMP maintainer in that case.

I feel currently I have exactly the same problem as an HMP maintainer,
since it seems impossible to get small changes in without tripping over
QMP.

> Avoiding QMP maintenance balloon out of control was one of our reasons
> for the rule "HMP commands must be implemented in terms of QMP".  We pay
> a modest amount of extra developer effort for a much-needed
> simplification of the maintainers' job.
> 
> Regarding "modest amount of effort": I already told you I'm prepared to
> back that claim with patches whenever requiring QMP blocks HMP patches.
> If that can't shake your conviction that requiring QMP is impractical, I
> figure nothing will.
> 
> Ways out of the impasse:
> 
> 1. Find a more capable maintainer for QMP (MAINTAINERS sections QMP and
> QAPI Schema).  To give you an idea of the work involved, almost 10% of
> the commits in 2.9 touched the latter.

I had considered asking the same question for HMP; but the only reason I
didn't is because I don't want HMP to die.

> 2. Split the work differently: make the HMP maintainer responsible for
> keeping QMP complete.
> 
> 3. Abandon QMP's goal to be complete.  Tell management applications to
> use HMP where QMP falls short.
> 
> To be brutally frank, 2. requires more trust in your willingness to care
> about QMP than I have right now.
> 
> By refusing to cooperate, you can force 3.
> 
> The solution most conducive to my personal well-being is probably 1.
> 
> Any other bright ideas?

IMHO the problem is that 'QMP be complete' is a fine goal but you're
applying it too strongly by not giving me the trust that I *DO* have the
willingness to care for QMP - but to be able to make reasonable
decisions for cases that really are debugging tools.

I strongly agree that all management tools should be using QMP, and
anything that they need should always be added to QMP (whether or
not they're also added to HMP, I'd prefer if they are, but I'm not
going to stop QMP only commands).

Look at this series; all it did was enable long standing existing HMP 'info'
commands to be used in the new context.  I chose only the info commands
because they looked like the type of thing developers would want to be
able to see the current state of qemu; I carefully did not enable
anything that changed the state.

How was any of that 'Abandoning QMP's goal to be complete'?

The way I see it, this series just fixes the HMP breakage in preconfig
mode - because I care that HMP doesn't decay.

HMP is for debugging and easy use;  my judgement is that if the
developer of a device/subsection wants a command to make their
life easier when debugging then I'm happy for them to add it to HMP.
I want it to be simple and easy for those devs to be able to add
that debug.

But I'll only allow that as an HMP command if it's clearly
a convenient debugging tool rather than implementing some new feature
that should be in QMP.

> Separate discussion, does not block this series.

Good; please let me know if there are any other changes needed in this
series.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] Abandon our QMP first policy? (was: [PATCH v3 5/7] hmp: Add info commands for preconfig)

[Eduardo Habkost]

On Fri, Jun 15, 2018 at 06:10:48PM +0200, Markus Armbruster wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> Gerd Hoffmann <kraxel@redhat.com> writes:
> >> 
> >> >   Hi,
> >> >
> >> >> > Now let's review the three commands:
> >> >> > 
> >> >> > * Gerd, why does "info usbhost" have no QMP equivalent?
> >> >
> >> > Works only when running qemu directly, in the libvirt sandbox qemu
> >> > hasn't the permissions needed to scan the host usb bus so that would be
> >> > rather pointless ...
> >> 
> >> I don't think this meets either of the two criteria:
> >> 
> >> * I meets "makes no sense in QMP" only if QMP implies "can't scan host
> >>   USB bus".  Libvirt implies it, but QMP doesn't imply libvirt; it's its
> >>   most important, not its sole user.
> >> 
> >> * It meets "of use only for human users" only if we're convinced it's of
> >>   no use to programs.
> >> 
> >>   To avoid speculation and endless arguments about what could or could
> >>   not be of use, we've always stuck to "when in doubt, assume it could
> >>   be of use".
> >> 
> >>   "Libvirt can't use it" falls short.
> >> 
> >>   "Any management application worth anything would deny QEMU the
> >>   capability to scan the USB host bus, and thus wouldn't be able to use
> >>   it" is exactly the argument we intended to avoid.
> >> 
> >> QMP falling short of completeness in relatively unimportant ways like
> >> this one isn't exactly terrible.  The most serious effect is probably
> >> serving as a bad example that leads to further arguments like this one.
> >> These are well worth avoiding, though.
> >
> > Markus:
> >   a) This is a separate discussion; info usbhost has been there for many
> > years;  this patch set doesn't change that.
> 
> Yes, it's separate.
> 
> >   b) From HMP, if someone wants to add a command like 'info usbhost' to
> > make their debugging of USB easy, then HMP is all for that and there's
> > no way I'm going to require QMP implementations for a debug command.
> 
> As I said before, said someone is welcome to make his case for why the
> command is of use only for human users in the commit message.
> 
> If the QMP maintainer gets a say in judging that case, good.
> 
> But if your "no way" means what I have to assume it means, namely "no
> way I'm taking no for an answer", then we're at an impasse.

Is this really an impasse?  Dave says he isn't going to require
QMP implementations of debug commands.  Isn't a debug command
just for humans, by definition?

(Which I think is true in the specific case of "info usbhost",
because it's really just a wrapper around libusb.  I'm 100% sure
QEMU doesn't need to provide a QMP interface for libusb.)



> 
> Evolving HMP without consideration for QMP's mission makes carrying out
> that mission harder.  Specifically, me having to watch for new HMP
> commands bypassing QMP, then judge and maybe debate whether that's okay
> is beyond my capacity.  The debate part in particular.  I expect to fail
> as a QMP maintainer in that case.

What's QMP mission, exactly?

Do we have any signs of HMP evolving without consideration for
QMP's mission, recently?

Would it help if the HMP maintainer makes sure commit messages or
comments document more clearly why a QMP equivalent is not
necessary?


> 
> Avoiding QMP maintenance balloon out of control was one of our reasons
> for the rule "HMP commands must be implemented in terms of QMP".  We pay
> a modest amount of extra developer effort for a much-needed
> simplification of the maintainers' job.

I don't understand why "HMP commands must be implemented in terms
of QMP" helps avoiding QMP maintenance to balloon out of control.
My impression is that it's the opposite.

I fail to see the benefits of that policy.  What exactly is the
goal here?


> 
> Regarding "modest amount of effort": I already told you I'm prepared to
> back that claim with patches whenever requiring QMP blocks HMP patches.
> If that can't shake your conviction that requiring QMP is impractical, I
> figure nothing will.
> 
> Ways out of the impasse:
> 
> 1. Find a more capable maintainer for QMP (MAINTAINERS sections QMP and
> QAPI Schema).  To give you an idea of the work involved, almost 10% of
> the commits in 2.9 touched the latter.
> 
> 2. Split the work differently: make the HMP maintainer responsible for
> keeping QMP complete.

This sounds like a reasonable way to split responsibilities, to
me.


> 
> 3. Abandon QMP's goal to be complete.  Tell management applications to
> use HMP where QMP falls short.

What "complete" means here?  Why would we ever tell management
applications to use HMP?  We can always implement new QMP
commands if management applications need them.


> 
> To be brutally frank, 2. requires more trust in your willingness to care
> about QMP than I have right now.
> 
> By refusing to cooperate, you can force 3.
> 
> The solution most conducive to my personal well-being is probably 1.
> 
> Any other bright ideas?
> 
> Separate discussion, does not block this series.

-- 
Eduardo

Re: [Qemu-devel] Abandon our QMP first policy? (was: [PATCH v3 5/7] hmp: Add info commands for preconfig)

[Gerd Hoffmann]

  Hi,

> (Which I think is true in the specific case of "info usbhost",
> because it's really just a wrapper around libusb.  I'm 100% sure
> QEMU doesn't need to provide a QMP interface for libusb.)

Agree.

I think we can settle that specific case by deprecating and removing
"info usbhost".  lsusb (comes with usbutils) can be used instead.

cheers,
  Gerd

Re: [Qemu-devel] Abandon our QMP first policy? (was: [PATCH v3 5/7] hmp: Add info commands for preconfig)

[Dr. David Alan Gilbert]

* Gerd Hoffmann (kraxel@redhat.com) wrote:
>   Hi,
> 
> > (Which I think is true in the specific case of "info usbhost",
> > because it's really just a wrapper around libusb.  I'm 100% sure
> > QEMU doesn't need to provide a QMP interface for libusb.)
> 
> Agree.
> 
> I think we can settle that specific case by deprecating and removing
> "info usbhost".  lsusb (comes with usbutils) can be used instead.

OK, I'll drop 'info usbhost' from this series, and we can remove it
completely sometime later.

Dave

> cheers,
>   Gerd
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK