Reenable hmp for preconfig mode

From: "Dr. David Alan Gilbert" <[email protected]>

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 <[email protected]>

---
 monitor.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

monitor.c +0 -0

index 6d0cec552e..f4a16e6a03 100644

monitor.c +13 -0

     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;

Quoted 12 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

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 <[email protected]>

---
 monitor.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

monitor.c typedef struct mon_cmd_t { · line 131

Quoted 9 lines

monitor.c +1 -0

index 6d0cec552e..f4a16e6a03 100644
@@ -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.

Quoted 34 lines

monitor.c +19 -0

     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;
+    }
 

monitor.c static const mon_cmd_t *monitor_parse_command(Monitor *mon, · line 3001

Quoted 2 lines

monitor.c +0 -0

     /* filter out following useless space */
     while (qemu_isspace(*p)) {

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

Regardless,

Reviewed-by Markus Armbruster <[email protected]>

From: "Dr. David Alan Gilbert" <[email protected]>

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

Signed-off-by Dr. David Alan Gilbert <[email protected]>

Reviewed-by Peter Xu <[email protected]>

Reviewed-by Igor Mammedov <[email protected]>

---
 hmp-commands.hx | 1 +
 monitor.c       | 8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

hmp-commands.hx +1 -0

index 0734fea931..8bf590ae4b 100644
@@ -15,6 +15,7 @@ ETEXI
         .params     = "[cmd]",
         .help       = "show the help",
         .cmd        = do_help_cmd,
+        .flags      = "p",
     },
 
 STEXI

monitor.c +4 -0

index f4a16e6a03..31c8f5dc88 100644
@@ -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]);
     }

monitor.c +0 -0

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

Quoted 42 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

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

Signed-off-by Dr. David Alan Gilbert <[email protected]>

Reviewed-by Peter Xu <[email protected]>

Reviewed-by Igor Mammedov <[email protected]>

---
 hmp-commands.hx | 1 +
 monitor.c       | 8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

hmp-commands.hx +1 -0

index 0734fea931..8bf590ae4b 100644
@@ -15,6 +15,7 @@ ETEXI
         .params     = "[cmd]",
         .help       = "show the help",
         .cmd        = do_help_cmd,
+        .flags      = "p",
     },
 
 STEXI

monitor.c +4 -0

index f4a16e6a03..31c8f5dc88 100644
@@ -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,
 

monitor.c static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds, · line 988

Quoted 6 lines

monitor.c +3 -1

     /* 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()?

monitor.c static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds, · line 991

Quoted 3 lines

monitor.c +0 -0

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

Quoted 44 lines · * Markus Armbruster ([email protected]) wrote:

Quoted 42 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

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

Signed-off-by Dr. David Alan Gilbert <[email protected]>

Reviewed-by Peter Xu <[email protected]>

Reviewed-by Igor Mammedov <[email protected]>

---
 hmp-commands.hx | 1 +
 monitor.c       | 8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

hmp-commands.hx +1 -0

index 0734fea931..8bf590ae4b 100644
@@ -15,6 +15,7 @@ ETEXI
         .params     = "[cmd]",
         .help       = "show the help",
         .cmd        = do_help_cmd,
+        .flags      = "p",
     },
 
 STEXI

monitor.c +4 -0

index f4a16e6a03..31c8f5dc88 100644
@@ -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,
 

Quoted 8 lines

monitor.c static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds, · line 988

Quoted 6 lines

monitor.c +3 -1

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

Quoted 3 lines

monitor.c static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds, · line 991

Quoted 3 lines

monitor.c +0 -0

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

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Quoted 54 lines · Dr. David Alan Gilbert

Quoted 44 lines · * Markus Armbruster ([email protected]) wrote:

Quoted 42 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

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

Signed-off-by Dr. David Alan Gilbert <[email protected]>

Reviewed-by Peter Xu <[email protected]>

Reviewed-by Igor Mammedov <[email protected]>

---
 hmp-commands.hx | 1 +
 monitor.c       | 8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

hmp-commands.hx +1 -0

index 0734fea931..8bf590ae4b 100644
@@ -15,6 +15,7 @@ ETEXI
         .params     = "[cmd]",
         .help       = "show the help",
         .cmd        = do_help_cmd,
+        .flags      = "p",
     },
 
 STEXI

monitor.c +4 -0

index f4a16e6a03..31c8f5dc88 100644
@@ -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,
 

Quoted 8 lines

monitor.c static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds, · line 988

Quoted 6 lines

monitor.c +3 -1

     /* 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()?

Quoted 2 lines

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

Quoted 7 lines

Dave

Quoted 3 lines

monitor.c static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds, · line 991

Quoted 3 lines

monitor.c +0 -0

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

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Quoted 74 lines · * Markus Armbruster ([email protected]) wrote:

Quoted 54 lines · Dr. David Alan Gilbert

Quoted 44 lines · * Markus Armbruster ([email protected]) wrote:

Quoted 42 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

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

Signed-off-by Dr. David Alan Gilbert <[email protected]>

Reviewed-by Peter Xu <[email protected]>

Reviewed-by Igor Mammedov <[email protected]>

---
 hmp-commands.hx | 1 +
 monitor.c       | 8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

hmp-commands.hx +1 -0

index 0734fea931..8bf590ae4b 100644
@@ -15,6 +15,7 @@ ETEXI
         .params     = "[cmd]",
         .help       = "show the help",
         .cmd        = do_help_cmd,
+        .flags      = "p",
     },
 
 STEXI

monitor.c +4 -0

index f4a16e6a03..31c8f5dc88 100644
@@ -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,
 

Quoted 8 lines

monitor.c static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds, · line 988

Quoted 6 lines

monitor.c +3 -1

     /* 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()?

Quoted 2 lines

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.

Quoted 4 lines

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.

From: "Dr. David Alan Gilbert" <[email protected]>

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

Signed-off-by Dr. David Alan Gilbert <[email protected]>

Reviewed-by Peter Xu <[email protected]>

Reviewed-by Igor Mammedov <[email protected]>

---
 monitor.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

monitor.c +4 -1

index 31c8f5dc88..c369b392db 100644
@@ -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);
+            }
         }

Quoted 25 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

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

Signed-off-by Dr. David Alan Gilbert <[email protected]>

Reviewed-by Peter Xu <[email protected]>

Reviewed-by Igor Mammedov <[email protected]>

---
 monitor.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

monitor.c +4 -1

index 31c8f5dc88..c369b392db 100644
@@ -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);
+            }
         }

monitor.c static void monitor_find_completion_by_table(Monitor *mon, · line 3967

Quoted 10 lines

monitor.c +3 -1

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

From: Igor Mammedov <[email protected]>

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 <[email protected]>

---
 qapi/char.json |  3 ++-
 qapi/misc.json | 12 +++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

qapi/char.json +0 -0

index ae19dcd1ed..60f31d83fc 100644

qapi/char.json +0 -0

 ##
 # @ChardevBackendInfo:

qapi/misc.json +0 -0

index f83a63a0ab..1be1728c0e 100644

Quoted 7 lines · Dr. David Alan Gilbert (git)

From: Igor Mammedov <[email protected]>

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 <[email protected]>

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.

Quoted 10 lines

---
 qapi/char.json |  3 ++-
 qapi/misc.json | 12 +++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

qapi/char.json +0 -0

index ae19dcd1ed..60f31d83fc 100644

qapi/char.json line 66

Quoted 6 lines

qapi/char.json +2 -1

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

Quoted 8 lines

qapi/char.json +0 -0

 ##
 # @ChardevBackendInfo:

qapi/misc.json +0 -0

index f83a63a0ab..1be1728c0e 100644

qapi/misc.json line 121

Quoted 6 lines

qapi/misc.json +2 -1

@@ -117,7 +117,8 @@
 #    }
 #
 ##
-{ 'command': 'query-version', 'returns': 'VersionInfo' }
+{ 'command': 'query-version', 'returns': 'VersionInfo',
+  'allow-preconfig': true }

Returns data fixed at compile time. Okay.

qapi/misc.json line 245

Quoted 9 lines

qapi/misc.json +1 -1

 ##
 # @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().

qapi/misc.json line 305

Quoted 8 lines

qapi/misc.json +1 -1

 ##
 # @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().

Quoted 1 line

qapi/misc.json line 715

Quoted 9 lines

##
# @EventInfo:

qapi/misc.json +2 -1

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

Feeling lazy... why is this one okay?

Quoted 10 lines

qapi/misc.json +1 -1

 ##
 # @BalloonInfo:

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

qapi/misc.json line 2910

Quoted 2 lines

qapi/misc.json +0 -0

 ##
 # @PCDIMMDeviceInfo:

And this one?

Quoted 87 lines · * Markus Armbruster ([email protected]) wrote:

Quoted 7 lines · Dr. David Alan Gilbert (git)

From: Igor Mammedov <[email protected]>

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 <[email protected]>

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.

Quoted 10 lines

---
 qapi/char.json |  3 ++-
 qapi/misc.json | 12 +++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

qapi/char.json +0 -0

index ae19dcd1ed..60f31d83fc 100644

qapi/char.json line 66

Quoted 6 lines

qapi/char.json +2 -1

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

Quoted 8 lines

qapi/char.json +0 -0

 ##
 # @ChardevBackendInfo:

qapi/misc.json +0 -0

index f83a63a0ab..1be1728c0e 100644

qapi/misc.json line 121

Quoted 6 lines

qapi/misc.json +2 -1

@@ -117,7 +117,8 @@
 #    }
 #
 ##
-{ 'command': 'query-version', 'returns': 'VersionInfo' }
+{ 'command': 'query-version', 'returns': 'VersionInfo',
+  'allow-preconfig': true }

Returns data fixed at compile time. Okay.

qapi/misc.json line 245

Quoted 9 lines

qapi/misc.json +1 -1

 ##
 # @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().

qapi/misc.json line 305

Quoted 8 lines

qapi/misc.json +1 -1

 ##
 # @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().

Quoted 1 line

Quoted 11 lines

qapi/misc.json line 715

Quoted 9 lines

##
# @EventInfo:

qapi/misc.json +2 -1

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

Quoted 10 lines

Quoted 10 lines

qapi/misc.json +1 -1

 ##
 # @BalloonInfo:

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

Quoted 4 lines

qapi/misc.json line 2910

Quoted 2 lines

qapi/misc.json +0 -0

 ##
 # @PCDIMMDeviceInfo:

And this one?

Same trick as query-iothreads.

From: "Dr. David Alan Gilbert" <[email protected]>

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 <[email protected]>

Quoted 6 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

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.

Quoted 2 lines

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

query-status looks okay to me.

Quoted 2 lines

history
is HMP specific

Yes.

Quoted 2 lines

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.

Quoted 1 line

Signed-off-by Dr. David Alan Gilbert <[email protected]>

Quoted 3 lines

Now let's review the three commands:

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

Hi,

Quoted 3 lines

Quoted 3 lines

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

Quoted 9 lines · Gerd Hoffmann

Hi,

Quoted 3 lines

Quoted 3 lines

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.

Quoted 31 lines · * Markus Armbruster ([email protected]) wrote:

Quoted 9 lines · Gerd Hoffmann

Hi,

Quoted 3 lines

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 32 lines · Dr. David Alan Gilbert

Quoted 31 lines · * Markus Armbruster ([email protected]) wrote:

Quoted 9 lines · Gerd Hoffmann

Hi,

Quoted 3 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 32 lines · Dr. David Alan Gilbert

Quoted 31 lines · * Markus Armbruster ([email protected]) wrote:

Quoted 9 lines · Gerd Hoffmann

Hi,

Quoted 3 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

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

Hi,

Quoted 3 lines

(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

Quoted 24 lines · * Markus Armbruster ([email protected]) wrote:

Quoted 6 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

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.

Quoted 2 lines

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

query-status looks okay to me.

Quoted 2 lines

history
is HMP specific

Yes.

Quoted 2 lines

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

Quoted 8 lines

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.

Quoted 16 lines

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

Quoted 2 lines

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.

Quoted 54 lines · Dr. David Alan Gilbert

Quoted 24 lines · * Markus Armbruster ([email protected]) wrote:

Quoted 6 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

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.

Quoted 2 lines

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

query-status looks okay to me.

Quoted 2 lines

history
is HMP specific

Yes.

Quoted 2 lines

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

Quoted 8 lines

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.

Quoted 16 lines

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

Quoted 7 lines

Quoted 2 lines

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.

Quoted 2 lines

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

Quoted 10 lines

Quoted 4 lines

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.

Quoted 2 lines

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

Quoted 3 lines

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.

On Tue, Jun 12, 2018 at 09:49:14AM +0100, Dr. David Alan Gilbert wrote:
[...]

Quoted 14 lines

Quoted 13 lines

Quoted 4 lines

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.

Quoted 2 lines

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

Quoted 3 lines

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.

Quoted 16 lines · Eduardo Habkost

On Tue, Jun 12, 2018 at 09:49:14AM +0100, Dr. David Alan Gilbert wrote:
[...]

Quoted 14 lines

Quoted 13 lines

Quoted 4 lines

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.

Quoted 2 lines

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

Quoted 8 lines

Quoted 3 lines

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.

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

Quoted 1 line · Markus Armbruster

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

Quoted 8 lines · On Mon, 11 Jun 2018 15:40:16 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 1 line · Markus Armbruster

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

Quoted 11 lines · Igor Mammedov

Quoted 8 lines · On Mon, 11 Jun 2018 15:40:16 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 1 line · Markus Armbruster

* 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: <[email protected]>
https://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg02176.html

Quoted 7 lines

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.

Quoted 2 lines

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

Quoted 8 lines · On Mon, 11 Jun 2018 15:40:16 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 1 line · Markus Armbruster

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

From: "Dr. David Alan Gilbert" <[email protected]>

Add the exit_preconfig command to return to normality.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

Reviewed-by Peter Xu <[email protected]>

Reviewed-by Igor Mammedov <[email protected]>

---
 hmp-commands.hx | 15 +++++++++++++++
 hmp.c           |  7 +++++++
 hmp.h           |  1 +
 3 files changed, 23 insertions(+)

hmp-commands.hx +5 -0

index dc82ed526f..40e2f6ca08 100644
@@ -55,6 +55,21 @@ STEXI
 @item q or quit
 @findex quit
 Quit the emulator.
+ETEXI
+
+    {
+        .name       = "exit_preconfig",
+        .args_type  = "",

hmp-commands.hx +0 -0

 ETEXI
 
     {

hmp.c +0 -0

index ef93f4878b..c7be6ed394 100644

hmp.c +0 -0

 void hmp_cpu(Monitor *mon, const QDict *qdict)
 {
     int64_t cpu_index;

hmp.h +1 -0

index 20f27439d3..33354f1bdd 100644
@@ -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);

Quoted 26 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

Add the exit_preconfig command to return to normality.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

Reviewed-by Peter Xu <[email protected]>

Reviewed-by Igor Mammedov <[email protected]>

---
 hmp-commands.hx | 15 +++++++++++++++
 hmp.c           |  7 +++++++
 hmp.h           |  1 +
 3 files changed, 23 insertions(+)

hmp-commands.hx +5 -0

index dc82ed526f..40e2f6ca08 100644
@@ -55,6 +55,21 @@ STEXI
 @item q or quit
 @findex quit
 Quit the emulator.
+ETEXI
+
+    {
+        .name       = "exit_preconfig",
+        .args_type  = "",

hmp-commands.hx STEXI · line 72

Quoted 10 lines

hmp-commands.hx +10 -0

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

Quoted 11 lines

hmp-commands.hx +0 -0

 ETEXI
 
     {

hmp.c +0 -0

index ef93f4878b..c7be6ed394 100644
@@ -1065,6 +1065,13 @@ void hmp_system_powerdown(Monitor *mon, const QDict *qdict)
     qmp_system_powerdown(NULL);
 }
 

hmp.c void hmp_system_powerdown(Monitor *mon, const QDict *qdict) · line 1074

Quoted 7 lines

hmp.c +7 -0

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

Quoted 7 lines

hmp.c +0 -0

 void hmp_cpu(Monitor *mon, const QDict *qdict)
 {
     int64_t cpu_index;

hmp.h +0 -0

index 20f27439d3..33354f1bdd 100644

hmp.h void hmp_quit(Monitor *mon, const QDict *qdict); · line 50

Quoted 8 lines

hmp.h +1 -0

@@ -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);

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

Quoted 1 line · * Markus Armbruster ([email protected]) wrote:

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

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

Quoted 5 lines · Dr. David Alan Gilbert

Quoted 1 line · * Markus Armbruster ([email protected]) 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.