Daniel P. Berrangé <berra...@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é <berra...@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,
> + },
