Re: [PATCH v3 13/19] qapi: introduce x-query-skeys QMP command

[Daniel P . Berrangé]

On Tue, Oct 12, 2021 at 09:12:23AM +0200, Thomas Huth wrote:
> On 30/09/2021 15.23, Daniel P. Berrangé wrote:
> > This is a counterpart to the HMP "info skeys" 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.
> > 
> > Including 'common.json' into 'machine-target.json' created a little
> > problem because the static marshalling method for HumanReadableText
> > is generated unconditionally. It is only used, however, conditionally
> > on certain target architectures.
> > 
> > To deal with this we change the QAPI code generator to simply mark
> > all static marshalling functions with G_GNUC_UNSED to hide the
> > compiler warning.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> ...
> > +void hmp_info_skeys(Monitor *mon, const QDict *qdict)
> > +{
> > +    Error *err = NULL;
> > +    g_autoptr(HumanReadableText) info = NULL;
> > +    uint64_t addr = qdict_get_int(qdict, "addr");
> > +
> > +    info = qmp_x_query_skeys(addr, &err);
> > +    if (err) {
> > +        error_report_err(err);
> 
> Shouldn't that rather be:
> 
>            monitor_printf(mon, "%s\n", error_get_pretty(err));
> 
> or something similar?

error_report_err gets (eventually) hooked into monitor_printf() if
current monitor in the thread is non-NULL

> 
> >           return;
> >       }
> > -    monitor_printf(mon, "  key: 0x%X\n", key);
> > +    monitor_printf(mon, "%s", info->human_readable_text);
> >   }
> 
> Apart the question above, patch looks fine to me.
> 
>  Thomas
> 

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