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

[Markus Armbruster]

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(-)
>
> diff --git a/docs/devel/writing-monitor-commands.rst 
> b/docs/devel/writing-monitor-commands.rst
> index 0f3b751dab..82a382d700 100644
> --- a/docs/devel/writing-monitor-commands.rst
> +++ b/docs/devel/writing-monitor-commands.rst
> @@ -375,7 +375,9 @@ best practices. An example where this approach is taken 
> is the QMP
>  command "x-query-registers". This returns a formatted dump of the
>  architecture specific CPU state. The way the data is formatted varies
>  across QEMU targets, is liable to change over time, and is only
> -intended to be consumed as an opaque string by machines.
> +intended to be consumed as an opaque string by machines. Refer to the
> +`Writing a debugging aid returning unstructured text`_ section for
> +an illustration.
>  
>  User Defined Types
>  ~~~~~~~~~~~~~~~~~~
> @@ -647,3 +649,86 @@ has to traverse the list, it's shown below for 
> reference::
>  
>       qapi_free_TimerAlarmMethodList(method_list);
>   }
> +
> +Writing a debugging aid returning unstructured text
> +---------------------------------------------------
> +
> +As discussed in section `Modelling data in QAPI`_, it is required that
> +commands expecting machine usage be using fine-grained QAPI data types.
> +The exception to this rule applies when the command is solely intended
> +as a debugging aid and allows for returning unstructured text. This is
> +commonly needed for query commands that report aspects of QEMU's
> +internal state that are useful to human operators.
> +
> +In this example we will consider a simplified variant of the HMP
> +command ``info roms``. Following the earlier rules, this command will
> +need to live under the ``x-`` name prefix, so its QMP implementation
> +will be called ``x-query-roms``. It will have no parameters and will
> +return a single text string::
> +
> + { 'struct': 'HumanReadableText',
> +   'data': { 'human-readable-text': 'str' } }
> +
> + { 'command': 'x-query-roms',
> +   'returns': 'HumanReadableText' }
> +
> +The ``HumanReadableText`` struct is intended to be used for all
> +commands, under the ``x-`` name prefix that are returning unstructured
> +text targetted at humans. It should never be used for commands outside
> +the ``x-`` name prefix, as those should be using structured QAPI types.

This clashes with my "[PATCH v2 0/9] Configurable policy for handling
unstable interfaces", which replaces "you must give unstable stuff names
starting with 'x-'" by "you must give unstable stuff feature flag
'unstable' (and may give it a name starting with 'x-')".

> +
> +Implementing the QMP command
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +The QMP implementation will typically involve creating a ``GString``
> +object and printing formatted data into it::
> +
> + HumanReadableText *qmp_x_query_roms(Error **errp)
> + {
> +     g_autoptr(GString) buf = g_string_new("");
> +     Rom *rom;
> +
> +     QTAILQ_FOREACH(rom, &roms, next) {
> +        g_string_append_printf("%s size=0x%06zx name=\"%s\"\n",
> +                               memory_region_name(rom->mr),
> +                               rom->romsize,
> +                               rom->name);
> +     }
> +
> +     return human_readable_text_from_str(buf);
> + }
> +
> +
> +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;
        }

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.

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.

> +     monitor_printf(mon, "%s\n", info->human_readable_text);

Sure you want to print an extra newline?

If not, then consider

        monitor_puts(mon, info->human_readable_text);

> + }
> +
> +Also, you have to add the function's prototype to the hmp.h file.
> +
> +There's one last step to actually make the command available to
> +monitor users, we should add it to the hmp-commands-info.hx file::
> +
> +    {
> +        .name       = "roms",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show roms",
> +        .cmd        = hmp_info_roms,
> +    },