Re: [PATCH 1/2] docs/devel/writing-monitor-commands: Repair a decade of rot

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On Tue, Feb 27, 2024 at 12:56:16PM +0100, Markus Armbruster wrote:
>> The tutorial doesn't match reality since at least 2013.  Repairing it
>> involves fixing the following issues:
>> 
>> * Update for commit 6d327171551 (aio / timers: Remove alarm timers):
>>   replace the broken examples.  Instead of having one for returning a
>>   struct and another for returning a list of structs, do just one for
>>   the latter.  This resolves the FIXME added in commit
>>   e218052f928 (aio / timers: De-document -clock) back in 2014.
>> 
>> * Update for commit 895a2a80e0e (qapi: Use 'struct' instead of 'type'
>>   in schema).
>> 
>> * Update for commit 3313b6124b5 (qapi: add qapi2texi script): add
>>   required documentation to the schema snippets, and drop section
>>   "Command Documentation".
>> 
>> * Update for commit a3c45b3e629 (qapi: New special feature flag
>>   "unstable"): supply the required feature, deemphasize the x- prefix.
>> 
>> * Update for commit dd98234c059 (qapi: introduce x-query-roms QMP
>>   command): rephrase from "add new command" to "examine existing
>>   command".
>> 
>> * Update for commit 9492718b7c0 (qapi misc: Elide redundant has_FOO in
>>   generated C): hello-world's message argument no longer comes with a
>>   has_message, add a second argument that does.
>> 
>> * Update for moved and renamed files.
>> 
>> While there, update QMP version output to current output.
>
> Nice cleanups.

Thanks!

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  docs/devel/writing-monitor-commands.rst | 460 ++++++++++--------------
>>  1 file changed, 181 insertions(+), 279 deletions(-)
>
>>  
>> -The "type" keyword defines a new QAPI type. Its "data" member contains the
>> -type's members. In this example our members are the "clock-name" and the
>> -"next-deadline" one, which is optional.
>> +The "struct" keyword defines a new QAPI type. Its "data" member
>> +contains the type's members. In this example our members are
>> +"filename" and "bootindex".  The latter is optional.
>
> Is there any rhyme or reason behind one vs. two spaces after sentences here?

Accident.  I habitually use two spaces, but this document uses just one.
I tried to be locally consistent, but habit won in this case.  I'll fix
it.

>>  
>>  The HMP command
>>  ~~~~~~~~~~~~~~~
>>  
>> -Here's the HMP counterpart of the query-alarm-clock command::
>> +Here's the HMP counterpart of the query-option-roms command::
>>  
>> - void hmp_info_alarm_clock(Monitor *mon)
>> + void hmp_info_option_roms(Monitor *mon, const QDict *qdict)
>>   {
>> -     QemuAlarmClock *clock;
>>       Error *err = NULL;
>> +     OptionRomInfoList *info_list, *tail;
>> +     OptionRomInfo *info;
>>  
>> -     clock = qmp_query_alarm_clock(&err);
>> +     info_list = qmp_query_option_roms(&err);
>>       if (hmp_handle_error(mon, err)) {
>> -         return;
>> +	 return;
>
> Was the change to TAB intentional?

Another accident.

>
>>       }
>>  
>> -     monitor_printf(mon, "Alarm clock method in use: '%s'\n", clock->clock_name);
>> -     if (clock->has_next_deadline) {
>> -         monitor_printf(mon, "Next alarm will fire in %" PRId64 " nanoseconds\n",
>> -                        clock->next_deadline);
>> +     for (tail = info_list; tail; tail = tail->next) {
>> +	 info = tail->value;
>> +	 monitor_printf(mon, "%s", info->filename);
>> +	 if (info->has_bootindex) {
>> +	     monitor_printf(mon, " %" PRId64, info->bootindex);
>> +	 }
>> +	 monitor_printf(mon, "\n");
>>       }
>
> More TABs.

Will expand them all.

>>  Writing a debugging aid returning unstructured text
>>  ---------------------------------------------------
> ...
>> -The ``HumanReadableText`` struct is intended to be used for all
>> -commands, under the ``x-`` name prefix that are returning unstructured
>> -text targeted at humans. It should never be used for commands outside
>> -the ``x-`` name prefix, as those should be using structured QAPI types.
>> +The ``HumanReadableText`` struct is defined in qapi/common.json as a
>> +struct with a string member. It is intended to be used for all
>> +commands that are returning unstructured text targeted at
>> +humans. These should all have feature 'unstable'.  Note that the
>> +feature's documentation states why the command is unstable.  WE
>
> We

Will fix.

>> +commonly use a ``x-`` command name prefix to make lack of stability
>> +obvious to human users.
>>  
>
> Cleanups are trivial enough that I'm fine with you making them before
> including:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!