Re: [PATCH v3 04/19] docs/devel: add example of command returning unstructured text

[Daniel P . Berrangé]

On Thu, Oct 28, 2021 at 04:47:14PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > This illustrates how to add a QMP command returning unstructured text,
> > following the guidelines added in the previous patch. The example uses
> > a simplified version of 'info roms'.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  docs/devel/writing-monitor-commands.rst | 87 ++++++++++++++++++++++++-
> >  1 file changed, 86 insertions(+), 1 deletion(-)

> > +Implementing the HMP command
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +Now that the QMP command is in place, we can also make it available in
> > +the human monitor (HMP) as shown in previous examples. The HMP
> > +implementations will all look fairly similar, as all they need do is
> > +invoke the QMP command and then print the resulting text or error
> > +message. Here's the implementation of the "info roms" HMP command::
> > +
> > + 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;
> > +     }
> 
> There's hmp_handle_error().
> 
> If it returned whether there's an error, we could write
> 
>         if (hmp_handle_error(err)) {
>             return;
>         }

I'll add a return value to hmp_handle_error and update existing
callers where appropriate

> 
> Of course, qmp_x_query_roms() can't fail, so we could just as well do
> 
>         g_autoptr(HumanReadableText) info = qmp_x_query_roms(&error_abort);
> 
> Okay as long as we show how to report errors in HMP commands
> *somewhere*, but the use of &error_abort needs explaining.  Not sure
> that's an improvement here.

For the sake of illustration I think I'll stick with hmp_handle_error
and not &error_abort.  In fact following Dave's feedback, I've added
a helper to provide the HMP implementation with no code required in
the no-arg case.

> Aside: the existing examples show questionable error handling.  The
> first one uses error_get_pretty() instead of hmp_handle_error().  The
> other two throw away the error they get from the QMP command, and report
> "Could not query ..." instead, which is a bit of an anti-pattern.

I'll fix those examples

> 
> > +     monitor_printf(mon, "%s\n", info->human_readable_text);
> 
> Sure you want to print an extra newline?

Opps, mistake.

> If not, then consider
> 
>         monitor_puts(mon, info->human_readable_text);

I'd prefer "%s", since it avoids danger of the human readable
text possibly containing a '%' sign that trips up printf.


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