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

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> 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

Thanks!

>> 
>> > +     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.

Read that again: "puts" :)