diff mbox series

[RFC,2/3] monitor: Allow passing HMP arguments to QMP HumanReadableText API

Message ID 20240610175852.21215-3-philmd@linaro.org
State New
Headers show
Series monitor: Pass HMP arguments to QMP HumanReadableText API as JSON | expand

Commit Message

Philippe Mathieu-Daudé June 10, 2024, 5:58 p.m. UTC
Allow HMP commands implemented using the HumanReadableText API
(via the HMPCommand::cmd_info_hrt handler) to pass arguments
to the QMP equivalent command. The arguments are serialized as
a JSON dictionary.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 docs/devel/writing-monitor-commands.rst | 15 ++++++++++++++-
 qapi/machine.json                       | 24 ++++++++++++++++++++++++
 include/monitor/monitor.h               |  3 ++-
 monitor/monitor-internal.h              |  2 +-
 accel/tcg/monitor.c                     |  4 ++--
 hw/core/loader.c                        |  2 +-
 hw/core/machine-qmp-cmds.c              |  9 +++++----
 hw/usb/bus.c                            |  2 +-
 monitor/hmp-target.c                    |  3 ++-
 monitor/hmp.c                           | 11 +++++++----
 10 files changed, 59 insertions(+), 16 deletions(-)

Comments

Daniel P. Berrangé June 10, 2024, 6:26 p.m. UTC | #1
On Mon, Jun 10, 2024 at 07:58:51PM +0200, Philippe Mathieu-Daudé wrote:
> Allow HMP commands implemented using the HumanReadableText API
> (via the HMPCommand::cmd_info_hrt handler) to pass arguments
> to the QMP equivalent command. The arguments are serialized as
> a JSON dictionary.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  docs/devel/writing-monitor-commands.rst | 15 ++++++++++++++-
>  qapi/machine.json                       | 24 ++++++++++++++++++++++++
>  include/monitor/monitor.h               |  3 ++-
>  monitor/monitor-internal.h              |  2 +-
>  accel/tcg/monitor.c                     |  4 ++--
>  hw/core/loader.c                        |  2 +-
>  hw/core/machine-qmp-cmds.c              |  9 +++++----
>  hw/usb/bus.c                            |  2 +-
>  monitor/hmp-target.c                    |  3 ++-
>  monitor/hmp.c                           | 11 +++++++----
>  10 files changed, 59 insertions(+), 16 deletions(-)
> 
> diff --git a/docs/devel/writing-monitor-commands.rst b/docs/devel/writing-monitor-commands.rst
> index 930da5cd06..843458e52c 100644
> --- a/docs/devel/writing-monitor-commands.rst
> +++ b/docs/devel/writing-monitor-commands.rst
> @@ -561,6 +561,7 @@ returns a ``HumanReadableText``::
>   # Since: 6.2
>   ##
>   { 'command': 'x-query-roms',
> +   'data': { 'json-args': 'str'},
>     'returns': 'HumanReadableText',
>     'features': [ 'unstable' ] }
>  
> @@ -578,7 +579,7 @@ Implementing the QMP command
>  The QMP implementation will typically involve creating a ``GString``
>  object and printing formatted data into it, like this::
>  
> - HumanReadableText *qmp_x_query_roms(Error **errp)
> + HumanReadableText *qmp_x_query_roms(const char *json_args, Error **errp)
>   {
>       g_autoptr(GString) buf = g_string_new("");
>       Rom *rom;
> @@ -596,6 +597,18 @@ object and printing formatted data into it, like this::
>  The actual implementation emits more information.  You can find it in
>  hw/core/loader.c.
>  
> +For QMP command taking (optional) parameters, these parameters are
> +serialized as a JSON dictionary, and can be retrieved using the QDict
> +API. If the previous ``x-query-roms`` command were taking a "index"
> +argument, it could be retrieved as::
> +
> + HumanReadableText *qmp_x_query_roms(const char *json_args, Error **errp)
> + {
> +     g_autoptr(GString) buf = g_string_new("");
> +     QDict *qdict = qobject_to(QDict, qobject_from_json(json_args, &error_abort));
> +     uint64_t index = qdict_get_int(qdict, "index");
> +     ...
> + }

Passing json inside json is pretty gross, and throwing away a key
benefit of QAPI - that it de-serializes the JSON into the actual
data types that you need, avoiding manual & error prone code for
unpacking args from a QDict.

IMHO if a commend requires arguments, they should be modelled
explicitly, and not use the  cmd_info_hrt convenience handler
which was only ever intended simple for no-arg 'info' commands.

With regards,
Daniel
diff mbox series

Patch

diff --git a/docs/devel/writing-monitor-commands.rst b/docs/devel/writing-monitor-commands.rst
index 930da5cd06..843458e52c 100644
--- a/docs/devel/writing-monitor-commands.rst
+++ b/docs/devel/writing-monitor-commands.rst
@@ -561,6 +561,7 @@  returns a ``HumanReadableText``::
  # Since: 6.2
  ##
  { 'command': 'x-query-roms',
+   'data': { 'json-args': 'str'},
    'returns': 'HumanReadableText',
    'features': [ 'unstable' ] }
 
@@ -578,7 +579,7 @@  Implementing the QMP command
 The QMP implementation will typically involve creating a ``GString``
 object and printing formatted data into it, like this::
 
- HumanReadableText *qmp_x_query_roms(Error **errp)
+ HumanReadableText *qmp_x_query_roms(const char *json_args, Error **errp)
  {
      g_autoptr(GString) buf = g_string_new("");
      Rom *rom;
@@ -596,6 +597,18 @@  object and printing formatted data into it, like this::
 The actual implementation emits more information.  You can find it in
 hw/core/loader.c.
 
+For QMP command taking (optional) parameters, these parameters are
+serialized as a JSON dictionary, and can be retrieved using the QDict
+API. If the previous ``x-query-roms`` command were taking a "index"
+argument, it could be retrieved as::
+
+ HumanReadableText *qmp_x_query_roms(const char *json_args, Error **errp)
+ {
+     g_autoptr(GString) buf = g_string_new("");
+     QDict *qdict = qobject_to(QDict, qobject_from_json(json_args, &error_abort));
+     uint64_t index = qdict_get_int(qdict, "index");
+     ...
+ }
 
 Implementing the HMP command
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/qapi/machine.json b/qapi/machine.json
index 1283d14493..6da72f2585 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1697,6 +1697,8 @@ 
 #
 # Query interrupt statistics
 #
+# @json-args: HMP arguments encoded as JSON string (unused for this command).
+#
 # Features:
 #
 # @unstable: This command is meant for debugging.
@@ -1706,6 +1708,7 @@ 
 # Since: 6.2
 ##
 { 'command': 'x-query-irq',
+  'data': { 'json-args': 'str'},
   'returns': 'HumanReadableText',
   'features': [ 'unstable' ] }
 
@@ -1714,6 +1717,8 @@ 
 #
 # Query TCG compiler statistics
 #
+# @json-args: HMP arguments encoded as JSON string (unused for this command).
+#
 # Features:
 #
 # @unstable: This command is meant for debugging.
@@ -1723,6 +1728,7 @@ 
 # Since: 6.2
 ##
 { 'command': 'x-query-jit',
+  'data': { 'json-args': 'str'},
   'returns': 'HumanReadableText',
   'if': 'CONFIG_TCG',
   'features': [ 'unstable' ] }
@@ -1732,6 +1738,8 @@ 
 #
 # Query NUMA topology information
 #
+# @json-args: HMP arguments encoded as JSON string (unused for this command).
+#
 # Features:
 #
 # @unstable: This command is meant for debugging.
@@ -1741,6 +1749,7 @@ 
 # Since: 6.2
 ##
 { 'command': 'x-query-numa',
+  'data': { 'json-args': 'str'},
   'returns': 'HumanReadableText',
   'features': [ 'unstable' ] }
 
@@ -1749,6 +1758,8 @@ 
 #
 # Query TCG opcode counters
 #
+# @json-args: HMP arguments encoded as JSON string (unused for this command).
+#
 # Features:
 #
 # @unstable: This command is meant for debugging.
@@ -1758,6 +1769,7 @@ 
 # Since: 6.2
 ##
 { 'command': 'x-query-opcount',
+  'data': { 'json-args': 'str'},
   'returns': 'HumanReadableText',
   'if': 'CONFIG_TCG',
   'features': [ 'unstable' ] }
@@ -1767,6 +1779,8 @@ 
 #
 # Query system ramblock information
 #
+# @json-args: HMP arguments encoded as JSON string (unused for this command).
+#
 # Features:
 #
 # @unstable: This command is meant for debugging.
@@ -1776,6 +1790,7 @@ 
 # Since: 6.2
 ##
 { 'command': 'x-query-ramblock',
+  'data': { 'json-args': 'str'},
   'returns': 'HumanReadableText',
   'features': [ 'unstable' ] }
 
@@ -1784,6 +1799,8 @@ 
 #
 # Query information on the registered ROMS
 #
+# @json-args: HMP arguments encoded as JSON string (unused for this command).
+#
 # Features:
 #
 # @unstable: This command is meant for debugging.
@@ -1793,6 +1810,7 @@ 
 # Since: 6.2
 ##
 { 'command': 'x-query-roms',
+  'data': { 'json-args': 'str'},
   'returns': 'HumanReadableText',
   'features': [ 'unstable' ] }
 
@@ -1801,6 +1819,8 @@ 
 #
 # Query information on the USB devices
 #
+# @json-args: HMP arguments encoded as JSON string (unused for this command).
+#
 # Features:
 #
 # @unstable: This command is meant for debugging.
@@ -1810,6 +1830,7 @@ 
 # Since: 6.2
 ##
 { 'command': 'x-query-usb',
+  'data': { 'json-args': 'str'},
   'returns': 'HumanReadableText',
   'features': [ 'unstable' ] }
 
@@ -1870,6 +1891,8 @@ 
 #
 # Query information on interrupt controller devices
 #
+# @json-args: HMP arguments encoded as JSON string (unused for this command).
+#
 # Features:
 #
 # @unstable: This command is meant for debugging.
@@ -1879,5 +1902,6 @@ 
 # Since: 9.1
 ##
 { 'command': 'x-query-interrupt-controllers',
+  'data': { 'json-args': 'str'},
   'returns': 'HumanReadableText',
   'features': [ 'unstable' ]}
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 965f5d5450..b21c702c12 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -58,7 +58,8 @@  int64_t monitor_fdset_dup_fd_find(int dup_fd);
 void monitor_register_hmp(const char *name, bool info,
                           void (*cmd)(Monitor *mon, const QDict *qdict));
 void monitor_register_hmp_info_hrt(const char *name,
-                                   HumanReadableText *(*handler)(Error **errp));
+                                   HumanReadableText *(*handler)(const char *json_args,
+                                                                 Error **errp));
 
 int error_vprintf_unless_qmp(const char *fmt, va_list ap) G_GNUC_PRINTF(1, 0);
 int error_printf_unless_qmp(const char *fmt, ...) G_GNUC_PRINTF(1, 2);
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 252de85681..b3aa50834b 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -81,7 +81,7 @@  typedef struct HMPCommand {
      * @cmd_info_hrt to the corresponding QMP handler that returns
      * the formatted text.
      */
-    HumanReadableText *(*cmd_info_hrt)(Error **errp);
+    HumanReadableText *(*cmd_info_hrt)(const char *json_args, Error **errp);
     bool coroutine;
     /*
      * @sub_table is a list of 2nd level of commands. If it does not exist,
diff --git a/accel/tcg/monitor.c b/accel/tcg/monitor.c
index 093efe9714..517c96eeb7 100644
--- a/accel/tcg/monitor.c
+++ b/accel/tcg/monitor.c
@@ -199,7 +199,7 @@  static void dump_exec_info(GString *buf)
     tcg_dump_info(buf);
 }
 
-HumanReadableText *qmp_x_query_jit(Error **errp)
+HumanReadableText *qmp_x_query_jit(const char *json_args, Error **errp)
 {
     g_autoptr(GString) buf = g_string_new("");
 
@@ -220,7 +220,7 @@  static void tcg_dump_op_count(GString *buf)
     g_string_append_printf(buf, "[TCG profiler not compiled]\n");
 }
 
-HumanReadableText *qmp_x_query_opcount(Error **errp)
+HumanReadableText *qmp_x_query_opcount(const char *json_args, Error **errp)
 {
     g_autoptr(GString) buf = g_string_new("");
 
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 2f8105d7de..e0da5edbb3 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1679,7 +1679,7 @@  void *rom_ptr_for_as(AddressSpace *as, hwaddr addr, size_t size)
     return cbdata.rom;
 }
 
-HumanReadableText *qmp_x_query_roms(Error **errp)
+HumanReadableText *qmp_x_query_roms(const char *json_args, Error **errp)
 {
     Rom *rom;
     g_autoptr(GString) buf = g_string_new("");
diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index 130217da8f..fc79772df8 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -214,7 +214,7 @@  MemdevList *qmp_query_memdev(Error **errp)
     return list;
 }
 
-HumanReadableText *qmp_x_query_numa(Error **errp)
+HumanReadableText *qmp_x_query_numa(const char *json_args, Error **errp)
 {
     g_autoptr(GString) buf = g_string_new("");
     int i, nb_numa_nodes;
@@ -311,7 +311,7 @@  MemoryInfo *qmp_query_memory_size_summary(Error **errp)
     return mem_info;
 }
 
-HumanReadableText *qmp_x_query_ramblock(Error **errp)
+HumanReadableText *qmp_x_query_ramblock(const char *json_args, Error **errp)
 {
     g_autoptr(GString) buf = ram_block_format();
 
@@ -351,7 +351,7 @@  static int qmp_x_query_irq_foreach(Object *obj, void *opaque)
     return 0;
 }
 
-HumanReadableText *qmp_x_query_irq(Error **errp)
+HumanReadableText *qmp_x_query_irq(const char *json_args, Error **errp)
 {
     g_autoptr(GString) buf = g_string_new("");
 
@@ -382,7 +382,8 @@  static int qmp_x_query_intc_foreach(Object *obj, void *opaque)
     return 0;
 }
 
-HumanReadableText *qmp_x_query_interrupt_controllers(Error **errp)
+HumanReadableText *qmp_x_query_interrupt_controllers(const char *json_args,
+                                                     Error **errp)
 {
     g_autoptr(GString) buf = g_string_new("");
     object_child_foreach_recursive(object_get_root(),
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index bfab2807d7..daa3f71d47 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -598,7 +598,7 @@  static char *usb_get_fw_dev_path(DeviceState *qdev)
     return fw_path;
 }
 
-HumanReadableText *qmp_x_query_usb(Error **errp)
+HumanReadableText *qmp_x_query_usb(const char *json_args, Error **errp)
 {
     g_autoptr(GString) buf = g_string_new("");
     USBBus *bus;
diff --git a/monitor/hmp-target.c b/monitor/hmp-target.c
index 0466474354..81e53a5767 100644
--- a/monitor/hmp-target.c
+++ b/monitor/hmp-target.c
@@ -157,7 +157,8 @@  void monitor_register_hmp(const char *name, bool info,
 }
 
 void monitor_register_hmp_info_hrt(const char *name,
-                                   HumanReadableText *(*handler)(Error **errp))
+                                   HumanReadableText *(*handler)(const char *json_args,
+                                                                 Error **errp))
 {
     HMPCommand *table = hmp_info_cmds;
 
diff --git a/monitor/hmp.c b/monitor/hmp.c
index 69c1b7e98a..7802a31412 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -29,6 +29,7 @@ 
 #include "monitor/hmp.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qnum.h"
+#include "qapi/qmp/qjson.h"
 #include "qemu/config-file.h"
 #include "qemu/ctype.h"
 #include "qemu/cutils.h"
@@ -1082,11 +1083,13 @@  fail:
     return NULL;
 }
 
-static void hmp_info_human_readable_text(Monitor *mon,
-                                         HumanReadableText *(*handler)(Error **))
+static void hmp_info_human_readable_text(Monitor *mon, QDict *qdict,
+                                         HumanReadableText *(*handler)(const char *,
+                                                                       Error **))
 {
     Error *err = NULL;
-    g_autoptr(HumanReadableText) info = handler(&err);
+    g_autoptr(GString) ret_json = qobject_to_json(QOBJECT(qdict));
+    g_autoptr(HumanReadableText) info = handler(ret_json->str, &err);
 
     if (hmp_handle_error(mon, err)) {
         return;
@@ -1100,7 +1103,7 @@  static void handle_hmp_command_exec(Monitor *mon,
                                     QDict *qdict)
 {
     if (cmd->cmd_info_hrt) {
-        hmp_info_human_readable_text(mon,
+        hmp_info_human_readable_text(mon, qdict,
                                      cmd->cmd_info_hrt);
     } else {
         cmd->cmd(mon, qdict);