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

[Daniel P . Berrangé]

On Mon, Oct 04, 2021 at 01:32:14PM +0100, Dr. David Alan Gilbert wrote:
> * 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?


This pattern is repeated in many, but not all, or the handlers in
this series, because I started with the easy cases of no-arg 'info'
commands. There's a few exceptions such as commands which drive off
the currently selected CPU state.  I'm not convince it is worth
adding specials to the hmp parser, since it will only be used for
a subset of the commands lng term. A helper function though could
do the job, since I've already introduced a helper for the QMP
side converting GString to HumanReadableText


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