Re: [PATCH v3 07/19] qapi: introduce x-query-roms QMP command

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> This is a counterpart to the HMP "info roms" 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: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  hw/core/loader.c            | 53 +++++++++++++++++++++++++------------
>  hw/core/machine-qmp-cmds.c  |  1 +
>  include/qapi/type-helpers.h | 14 ++++++++++
>  qapi/common.json            | 11 ++++++++
>  qapi/machine.json           | 12 +++++++++
>  qapi/meson.build            |  3 +++
>  qapi/qapi-type-helpers.c    | 23 ++++++++++++++++
>  7 files changed, 100 insertions(+), 17 deletions(-)
>  create mode 100644 include/qapi/type-helpers.h
>  create mode 100644 qapi/qapi-type-helpers.c
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index c623318b73..5ebdca3087 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -46,6 +46,8 @@
>  #include "qemu-common.h"
>  #include "qemu/datadir.h"
>  #include "qapi/error.h"
> +#include "qapi/qapi-commands-machine.h"
> +#include "qapi/type-helpers.h"
>  #include "trace.h"
>  #include "hw/hw.h"
>  #include "disas/disas.h"
> @@ -1472,32 +1474,49 @@ void *rom_ptr_for_as(AddressSpace *as, hwaddr addr, size_t size)
>      return cbdata.rom;
>  }
>  
> -void hmp_info_roms(Monitor *mon, const QDict *qdict)
> +HumanReadableText *qmp_x_query_roms(Error **errp)
>  {
>      Rom *rom;
> +    g_autoptr(GString) buf = g_string_new("");
>  
>      QTAILQ_FOREACH(rom, &roms, next) {
>          if (rom->mr) {
> -            monitor_printf(mon, "%s"
> -                           " size=0x%06zx name=\"%s\"\n",
> -                           memory_region_name(rom->mr),
> -                           rom->romsize,
> -                           rom->name);
> +            g_string_append_printf(buf, "%s"
> +                                   " size=0x%06zx name=\"%s\"\n",
> +                                   memory_region_name(rom->mr),
> +                                   rom->romsize,
> +                                   rom->name);
>          } else if (!rom->fw_file) {
> -            monitor_printf(mon, "addr=" TARGET_FMT_plx
> -                           " size=0x%06zx mem=%s name=\"%s\"\n",
> -                           rom->addr, rom->romsize,
> -                           rom->isrom ? "rom" : "ram",
> -                           rom->name);
> +            g_string_append_printf(buf, "addr=" TARGET_FMT_plx
> +                                   " size=0x%06zx mem=%s name=\"%s\"\n",
> +                                   rom->addr, rom->romsize,
> +                                   rom->isrom ? "rom" : "ram",
> +                                   rom->name);
>          } else {
> -            monitor_printf(mon, "fw=%s/%s"
> -                           " size=0x%06zx name=\"%s\"\n",
> -                           rom->fw_dir,
> -                           rom->fw_file,
> -                           rom->romsize,
> -                           rom->name);
> +            g_string_append_printf(buf, "fw=%s/%s"
> +                                   " size=0x%06zx name=\"%s\"\n",
> +                                   rom->fw_dir,
> +                                   rom->fw_file,
> +                                   rom->romsize,
> +                                   rom->name);
>          }
>      }
> +
> +    return human_readable_text_from_str(buf);
> +}
> +
> +
> +void hmp_info_roms(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    g_autoptr(HumanReadableText) info = qmp_x_query_roms(&err);
> +
> +    if (err) {
> +        error_report_err(err);
> +        return;
> +    }
> +
> +    monitor_printf(mon, "%s", info->human_readable_text);

This is getting copied in each one of these; it looks like you need
either:
  a) A helper function like:
       void hmp_info_from_qmp(Monitor *mon, HumanReadableText *(void)func) 
       {
           ...
       }

  b) Or teach the hmp parser to do the calls?

Dave

>  }
>  
>  typedef enum HexRecord HexRecord;
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index 216fdfaf3a..76f2b84d81 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -15,6 +15,7 @@
>  #include "qapi/qmp/qerror.h"
>  #include "qapi/qmp/qobject.h"
>  #include "qapi/qobject-input-visitor.h"
> +#include "qapi/type-helpers.h"
>  #include "qemu/main-loop.h"
>  #include "qom/qom-qobject.h"
>  #include "sysemu/hostmem.h"
> diff --git a/include/qapi/type-helpers.h b/include/qapi/type-helpers.h
> new file mode 100644
> index 0000000000..be1f181526
> --- /dev/null
> +++ b/include/qapi/type-helpers.h
> @@ -0,0 +1,14 @@
> +/*
> + * QAPI common helper functions
> + *
> + * This file provides helper functions related to types defined
> + * in the QAPI schema.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#include "qapi/qapi-types-common.h"
> +
> +HumanReadableText *human_readable_text_from_str(GString *str);
> diff --git a/qapi/common.json b/qapi/common.json
> index 7c976296f0..412cc4f5ae 100644
> --- a/qapi/common.json
> +++ b/qapi/common.json
> @@ -197,3 +197,14 @@
>  { 'enum': 'GrabToggleKeys',
>    'data': [ 'ctrl-ctrl', 'alt-alt', 'shift-shift','meta-meta', 'scrolllock',
>              'ctrl-scrolllock' ] }
> +
> +##
> +# @HumanReadableText:
> +#
> +# @human-readable-text: Formatted output intended for humans.
> +#
> +# Since: 6.2
> +#
> +##
> +{ 'struct': 'HumanReadableText',
> +  'data': { 'human-readable-text': 'str' } }
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 32d47f4e35..4c18904521 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1346,3 +1346,15 @@
>       '*cores': 'int',
>       '*threads': 'int',
>       '*maxcpus': 'int' } }
> +
> +##
> +# @x-query-roms:
> +#
> +# Query information on the registered ROMS
> +#
> +# Returns: registered ROMs
> +#
> +# Since: 6.2
> +##
> +{ 'command': 'x-query-roms',
> +  'returns': 'HumanReadableText' }
> diff --git a/qapi/meson.build b/qapi/meson.build
> index c356a385e3..c0c49c15e4 100644
> --- a/qapi/meson.build
> +++ b/qapi/meson.build
> @@ -10,6 +10,9 @@ util_ss.add(files(
>    'string-input-visitor.c',
>    'string-output-visitor.c',
>  ))
> +if have_system
> +  util_ss.add(files('qapi-type-helpers.c'))
> +endif
>  if have_system or have_tools
>    util_ss.add(files(
>      'qmp-dispatch.c',
> diff --git a/qapi/qapi-type-helpers.c b/qapi/qapi-type-helpers.c
> new file mode 100644
> index 0000000000..f76b34f647
> --- /dev/null
> +++ b/qapi/qapi-type-helpers.c
> @@ -0,0 +1,23 @@
> +/*
> + * QAPI common helper functions
> + *
> + * This file provides helper functions related to types defined
> + * in the QAPI schema.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qapi/type-helpers.h"
> +
> +HumanReadableText *human_readable_text_from_str(GString *str)
> +{
> +    HumanReadableText *ret = g_new0(HumanReadableText, 1);
> +
> +    ret->human_readable_text = g_steal_pointer(&str->str);
> +
> +    return ret;
> +}
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK