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

["Daniel P. Berrangé" <berrange@redhat.com>]

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