[RFC PATCH 0/3] monitor: Pass HMP arguments to QMP HumanReadableText API as JSON

[RFC PATCH 0/3] monitor: Pass HMP arguments to QMP HumanReadableText API as JSON

[Philippe Mathieu-Daudé]

Current HMPCommand::cmd_info_hrt() handlers don't allow
passing arguments from the monitor. This series pass them
to the underlying QMP commands as a JSON dictionary,
easily deserialized as QDict, similarly to how current
HMP commands receive their arguments. Thus very few
changes are required to port to the new API. As an
example, the @x-query-s390x-cmma command is ported.

Based-on: <20240610063518.50680-1-philmd@linaro.org>

Philippe Mathieu-Daudé (3):
  hw/s390x: Declare target specific monitor commands in hmp-target.h
  monitor: Allow passing HMP arguments to QMP HumanReadableText API
  hw/s390x: Introduce x-query-s390x-cmma QMP command

 docs/devel/writing-monitor-commands.rst | 15 ++++++++-
 qapi/machine.json                       | 44 +++++++++++++++++++++++++
 include/hw/s390x/storage-attributes.h   |  4 ---
 include/hw/s390x/storage-keys.h         |  4 ---
 include/monitor/hmp-target.h            |  5 +++
 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/s390x/s390-skeys.c                   |  2 ++
 hw/s390x/s390-stattrib.c                | 30 +++++++++++------
 hw/usb/bus.c                            |  2 +-
 monitor/hmp-target.c                    |  8 ++---
 monitor/hmp.c                           | 11 ++++---
 hmp-commands-info.hx                    |  2 +-
 16 files changed, 107 insertions(+), 40 deletions(-)

-- 
2.41.0

[PATCH 1/3] hw/s390x: Declare target specific monitor commands in hmp-target.h

[Philippe Mathieu-Daudé]

"monitor/hmp-target.h" is meant to hold target-specific commands.
Move s390x specific commands there, slightly simplifying hmp-target.c.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/s390x/storage-attributes.h | 4 ----
 include/hw/s390x/storage-keys.h       | 4 ----
 include/monitor/hmp-target.h          | 5 +++++
 hw/s390x/s390-skeys.c                 | 2 ++
 hw/s390x/s390-stattrib.c              | 2 ++
 monitor/hmp-target.c                  | 5 -----
 6 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/include/hw/s390x/storage-attributes.h b/include/hw/s390x/storage-attributes.h
index 8921a04d51..4916c75936 100644
--- a/include/hw/s390x/storage-attributes.h
+++ b/include/hw/s390x/storage-attributes.h
@@ -13,7 +13,6 @@
 #define S390_STORAGE_ATTRIBUTES_H
 
 #include "hw/qdev-core.h"
-#include "monitor/monitor.h"
 #include "qom/object.h"
 
 #define TYPE_S390_STATTRIB "s390-storage_attributes"
@@ -73,7 +72,4 @@ static inline Object *kvm_s390_stattrib_create(void)
 }
 #endif
 
-void hmp_info_cmma(Monitor *mon, const QDict *qdict);
-void hmp_migrationmode(Monitor *mon, const QDict *qdict);
-
 #endif /* S390_STORAGE_ATTRIBUTES_H */
diff --git a/include/hw/s390x/storage-keys.h b/include/hw/s390x/storage-keys.h
index aa2ec2aae5..1d9b7ead44 100644
--- a/include/hw/s390x/storage-keys.h
+++ b/include/hw/s390x/storage-keys.h
@@ -13,7 +13,6 @@
 #define S390_STORAGE_KEYS_H
 
 #include "hw/qdev-core.h"
-#include "monitor/monitor.h"
 #include "qom/object.h"
 
 #define TYPE_S390_SKEYS "s390-skeys"
@@ -114,7 +113,4 @@ void s390_skeys_init(void);
 
 S390SKeysState *s390_get_skeys_device(void);
 
-void hmp_dump_skeys(Monitor *mon, const QDict *qdict);
-void hmp_info_skeys(Monitor *mon, const QDict *qdict);
-
 #endif /* S390_STORAGE_KEYS_H */
diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index b679aaebbf..024cff0052 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -61,4 +61,9 @@ void hmp_gva2gpa(Monitor *mon, const QDict *qdict);
 void hmp_gpa2hva(Monitor *mon, const QDict *qdict);
 void hmp_gpa2hpa(Monitor *mon, const QDict *qdict);
 
+void hmp_dump_skeys(Monitor *mon, const QDict *qdict);
+void hmp_info_skeys(Monitor *mon, const QDict *qdict);
+void hmp_info_cmma(Monitor *mon, const QDict *qdict);
+void hmp_migrationmode(Monitor *mon, const QDict *qdict);
+
 #endif /* MONITOR_HMP_TARGET_H */
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 5c535d483e..7b2ccb94a5 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -23,6 +23,8 @@
 #include "sysemu/kvm.h"
 #include "migration/qemu-file-types.h"
 #include "migration/register.h"
+#include "monitor/hmp-target.h"
+#include "monitor/monitor.h"
 
 #define S390_SKEYS_BUFFER_SIZE (128 * KiB)  /* Room for 128k storage keys */
 #define S390_SKEYS_SAVE_FLAG_EOS 0x01
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index c4259b5327..9b4b8d8d0c 100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -19,6 +19,8 @@
 #include "exec/ram_addr.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qdict.h"
+#include "monitor/hmp-target.h"
+#include "monitor/monitor.h"
 #include "cpu.h"
 
 /* 512KiB cover 2GB of guest memory */
diff --git a/monitor/hmp-target.c b/monitor/hmp-target.c
index 1eb72ac1bf..0466474354 100644
--- a/monitor/hmp-target.c
+++ b/monitor/hmp-target.c
@@ -36,11 +36,6 @@
 #include "qapi/error.h"
 #include "qemu/cutils.h"
 
-#if defined(TARGET_S390X)
-#include "hw/s390x/storage-keys.h"
-#include "hw/s390x/storage-attributes.h"
-#endif
-
 /* Make devices configuration available for use in hmp-commands*.hx templates */
 #include CONFIG_DEVICES
 
-- 
2.41.0

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

[Philippe Mathieu-Daudé]

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");
+     ...
+ }
 
 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);
-- 
2.41.0

[RFC PATCH 3/3] hw/s390x: Introduce x-query-s390x-cmma QMP command

[Philippe Mathieu-Daudé]

This is a counterpart to the HMP "info cmma" command. It is being
added with an "x-" prefix because this QMP command is intended as an
adhoc debugging tool and will thus not be modelled in QAPI as fully
structured data, nor will it have long term guaranteed stability.
The existing HMP command is rewritten to call the QMP command.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 qapi/machine.json        | 20 ++++++++++++++++++++
 hw/s390x/s390-stattrib.c | 28 ++++++++++++++++++----------
 hmp-commands-info.hx     |  2 +-
 3 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index 6da72f2585..a56b7572b1 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1905,3 +1905,23 @@
   'data': { 'json-args': 'str'},
   'returns': 'HumanReadableText',
   'features': [ 'unstable' ]}
+
+##
+# @x-query-s390x-cmma:
+#
+# Query information on s390x CMMA storage attributes
+#
+# @json-args: HMP arguments encoded as JSON string.
+#
+# Features:
+#
+# @unstable: This command is meant for debugging.
+#
+# Returns: s390x CMMA storage attributes information
+#
+# Since: 9.1
+##
+{ 'command': 'x-query-s390x-cmma',
+  'data': { 'json-args': 'str'},
+  'returns': 'HumanReadableText',
+  'features': [ 'unstable' ]}
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index 9b4b8d8d0c..8c2372bd71 100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -19,6 +19,9 @@
 #include "exec/ram_addr.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qdict.h"
+#include "qapi/qapi-commands-machine.h"
+#include "qapi/qmp/qjson.h"
+#include "qapi/type-helpers.h"
 #include "monitor/hmp-target.h"
 #include "monitor/monitor.h"
 #include "cpu.h"
@@ -73,10 +76,12 @@ void hmp_migrationmode(Monitor *mon, const QDict *qdict)
     }
 }
 
-void hmp_info_cmma(Monitor *mon, const QDict *qdict)
+HumanReadableText *qmp_x_query_s390x_cmma(const char *json_args, Error **errp)
 {
+    g_autoptr(GString) buf = g_string_new("");
     S390StAttribState *sas = s390_get_stattrib_device();
     S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
+    QDict *qdict = qobject_to(QDict, qobject_from_json(json_args, &error_abort));
     uint64_t addr = qdict_get_int(qdict, "addr");
     uint64_t buflen = qdict_get_try_int(qdict, "count", 8);
     uint8_t *vals;
@@ -84,30 +89,33 @@ void hmp_info_cmma(Monitor *mon, const QDict *qdict)
 
     vals = g_try_malloc(buflen);
     if (!vals) {
-        monitor_printf(mon, "Error: %s\n", strerror(errno));
-        return;
+        error_setg(errp, "Failed to allocate memory");
+        return NULL;
     }
 
     len = sac->peek_stattr(sas, addr / TARGET_PAGE_SIZE, buflen, vals);
     if (len < 0) {
-        monitor_printf(mon, "Error: %s", strerror(-len));
+        error_setg_errno(errp, -len, "Could not get attributes");
         goto out;
     }
 
-    monitor_printf(mon, "  CMMA attributes, "
-                   "pages %" PRIu64 "+%d (0x%" PRIx64 "):\n",
-                   addr / TARGET_PAGE_SIZE, len, addr & ~TARGET_PAGE_MASK);
+    g_string_append_printf(buf, "  CMMA attributes, "
+                           "pages %" PRIu64 "+%d (0x%" PRIx64 "):\n",
+                           addr / TARGET_PAGE_SIZE, len,
+                           addr & ~TARGET_PAGE_MASK);
     for (cx = 0; cx < len; cx++) {
         if (cx % 8 == 7) {
-            monitor_printf(mon, "%02x\n", vals[cx]);
+            g_string_append_printf(buf, "%02x\n", vals[cx]);
         } else {
-            monitor_printf(mon, "%02x", vals[cx]);
+            g_string_append_printf(buf, "%02x", vals[cx]);
         }
     }
-    monitor_printf(mon, "\n");
+    g_string_append_c(buf, '\n');
 
 out:
+    qobject_unref(qdict);
     g_free(vals);
+    return human_readable_text_from_str(buf);
 }
 
 /* Migration support: */
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index cfd4ad5651..0a944e43ce 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -720,7 +720,7 @@ ERST
         .args_type  = "addr:l,count:l?",
         .params     = "address [count]",
         .help       = "Display the values of the CMMA storage attributes for a range of pages",
-        .cmd        = hmp_info_cmma,
+        .cmd_info_hrt = qmp_x_query_s390x_cmma,
     },
 #endif
 
-- 
2.41.0

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

[Daniel P. Berrangé]

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
-- 
|: 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: [PATCH 1/3] hw/s390x: Declare target specific monitor commands in hmp-target.h

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@linaro.org) wrote:
> "monitor/hmp-target.h" is meant to hold target-specific commands.
> Move s390x specific commands there, slightly simplifying hmp-target.c.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/s390x/storage-attributes.h | 4 ----
>  include/hw/s390x/storage-keys.h       | 4 ----
>  include/monitor/hmp-target.h          | 5 +++++
>  hw/s390x/s390-skeys.c                 | 2 ++
>  hw/s390x/s390-stattrib.c              | 2 ++
>  monitor/hmp-target.c                  | 5 -----
>  6 files changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/include/hw/s390x/storage-attributes.h b/include/hw/s390x/storage-attributes.h
> index 8921a04d51..4916c75936 100644
> --- a/include/hw/s390x/storage-attributes.h
> +++ b/include/hw/s390x/storage-attributes.h
> @@ -13,7 +13,6 @@
>  #define S390_STORAGE_ATTRIBUTES_H
>  
>  #include "hw/qdev-core.h"
> -#include "monitor/monitor.h"
>  #include "qom/object.h"
>  
>  #define TYPE_S390_STATTRIB "s390-storage_attributes"
> @@ -73,7 +72,4 @@ static inline Object *kvm_s390_stattrib_create(void)
>  }
>  #endif
>  
> -void hmp_info_cmma(Monitor *mon, const QDict *qdict);
> -void hmp_migrationmode(Monitor *mon, const QDict *qdict);
> -
>  #endif /* S390_STORAGE_ATTRIBUTES_H */
> diff --git a/include/hw/s390x/storage-keys.h b/include/hw/s390x/storage-keys.h
> index aa2ec2aae5..1d9b7ead44 100644
> --- a/include/hw/s390x/storage-keys.h
> +++ b/include/hw/s390x/storage-keys.h
> @@ -13,7 +13,6 @@
>  #define S390_STORAGE_KEYS_H
>  
>  #include "hw/qdev-core.h"
> -#include "monitor/monitor.h"
>  #include "qom/object.h"
>  
>  #define TYPE_S390_SKEYS "s390-skeys"
> @@ -114,7 +113,4 @@ void s390_skeys_init(void);
>  
>  S390SKeysState *s390_get_skeys_device(void);
>  
> -void hmp_dump_skeys(Monitor *mon, const QDict *qdict);
> -void hmp_info_skeys(Monitor *mon, const QDict *qdict);
> -
>  #endif /* S390_STORAGE_KEYS_H */
> diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
> index b679aaebbf..024cff0052 100644
> --- a/include/monitor/hmp-target.h
> +++ b/include/monitor/hmp-target.h
> @@ -61,4 +61,9 @@ void hmp_gva2gpa(Monitor *mon, const QDict *qdict);
>  void hmp_gpa2hva(Monitor *mon, const QDict *qdict);
>  void hmp_gpa2hpa(Monitor *mon, const QDict *qdict);
>  
> +void hmp_dump_skeys(Monitor *mon, const QDict *qdict);
> +void hmp_info_skeys(Monitor *mon, const QDict *qdict);
> +void hmp_info_cmma(Monitor *mon, const QDict *qdict);
> +void hmp_migrationmode(Monitor *mon, const QDict *qdict);
> +

Could you please add a comment here saying that these are all s390,
since it's not obvious from their names.
(and if we're lucky the other s390 commands will stay with them).

Dave

>  #endif /* MONITOR_HMP_TARGET_H */
> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
> index 5c535d483e..7b2ccb94a5 100644
> --- a/hw/s390x/s390-skeys.c
> +++ b/hw/s390x/s390-skeys.c
> @@ -23,6 +23,8 @@
>  #include "sysemu/kvm.h"
>  #include "migration/qemu-file-types.h"
>  #include "migration/register.h"
> +#include "monitor/hmp-target.h"
> +#include "monitor/monitor.h"
>  
>  #define S390_SKEYS_BUFFER_SIZE (128 * KiB)  /* Room for 128k storage keys */
>  #define S390_SKEYS_SAVE_FLAG_EOS 0x01
> diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
> index c4259b5327..9b4b8d8d0c 100644
> --- a/hw/s390x/s390-stattrib.c
> +++ b/hw/s390x/s390-stattrib.c
> @@ -19,6 +19,8 @@
>  #include "exec/ram_addr.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qdict.h"
> +#include "monitor/hmp-target.h"
> +#include "monitor/monitor.h"
>  #include "cpu.h"
>  
>  /* 512KiB cover 2GB of guest memory */
> diff --git a/monitor/hmp-target.c b/monitor/hmp-target.c
> index 1eb72ac1bf..0466474354 100644
> --- a/monitor/hmp-target.c
> +++ b/monitor/hmp-target.c
> @@ -36,11 +36,6 @@
>  #include "qapi/error.h"
>  #include "qemu/cutils.h"
>  
> -#if defined(TARGET_S390X)
> -#include "hw/s390x/storage-keys.h"
> -#include "hw/s390x/storage-attributes.h"
> -#endif
> -
>  /* Make devices configuration available for use in hmp-commands*.hx templates */
>  #include CONFIG_DEVICES
>  
> -- 
> 2.41.0
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/