Re: [PATCH v2 04/53] 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 | 85 +++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
>
> diff --git a/docs/devel/writing-monitor-commands.rst 
> b/docs/devel/writing-monitor-commands.rst
> index d68c552fdd..4cf51ab557 100644
> --- a/docs/devel/writing-monitor-commands.rst
> +++ b/docs/devel/writing-monitor-commands.rst
> @@ -647,3 +647,88 @@ has to traverse the list, it's shown below for 
> reference::
   Modelling data in QAPI
   ~~~~~~~~~~~~~~~~~~~~~~

   For a QMP command that to be considered stable and supported long term,
   there is a requirement returned data should be explicitly modelled
   using fine-grained QAPI types. As a general guide, a caller of the QMP
   command should never need to parse individual returned data fields. If
   a field appears to need parsing, then it should be split into separate
   fields corresponding to each distinct data item. This should be the
   common case for any new QMP command that is intended to be used by
   machines, as opposed to exclusively human operators.

   Some QMP commands, however, are only intended as ad hoc debugging aids
   for human operators. While they may return large amounts of formatted
   data, it is not expected that machines will need to parse the result.
   The overhead of defining a fine grained QAPI type for the data may not
   be justified by the potential benefit. In such cases, it is permitted
   to have a command return a simple string that contains formatted data,
   however, it is mandatory for the command to use the 'x-' name prefix.
   This indicates that the command is not guaranteed to be long term
   stable / liable to change in future and is not following QAPI design
   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.

Recommend to add a forward reference to section "Writing a debugging aid
..." here.

[...]
>  
>       qapi_free_TimerAlarmMethodList(method_list);
>   }
> +
> +Writing a debugging aid returning unstructured text
> +---------------------------------------------------
> +
> +As discussed at the start of the previous example, it is required that

Suggest 'As discussed in section "Modelling data in QAPI"'.

> +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.
> +
> +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)
> + {
> +     GString buf = g_string_new("");
> +     HumanReadableText ret = g_new0(HumanReadableText, 1);
> +     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);
> +     }
> +
> +     ret->human_readable_text = g_string_free(buf, FALSE);
> +
> +     return ret;
> + }
> +
> +
> +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);

Humor me: blank line between declarations and statements, please.

> +     if (err) {
> +         error_report_err(err);
> +         return;
> +     }
> +     monitor_printf(mon, "%s\n", 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,
> +    },