Daniel P. Berrangé <[email protected]> writes: > On Thu, Oct 28, 2021 at 04:47:14PM +0200, Markus Armbruster wrote: >> Daniel P. Berrangé <[email protected]> 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é <[email protected]> >> > --- >> > 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" :)
