Re: [PATCH v2 1/1] monitor/hmp: print trace as option in help for log command

[Markus Armbruster <armbru@redhat.com>]

Dongli Zhang <dongli.zhang@oracle.com> writes:

> Hi Markus,
>
> On 8/30/22 4:04 AM, Markus Armbruster wrote:
>> Dongli Zhang <dongli.zhang@oracle.com> writes:
>> 
>>> The below is printed when printing help information in qemu-system-x86_64
>>> command line, and when CONFIG_TRACE_LOG is enabled:
>>>
>>> $ qemu-system-x86_64 -d help
>>> ... ...
>>> trace:PATTERN   enable trace events
>>>
>>> Use "-d trace:help" to get a list of trace events.
>>>
>>> However, they are not printed in hmp "help log" command.
>> 
>> This leaves me guessing what exactly the patch tries to do.
>
> I will clarify in the commit message.
>
>> 
>>> Cc: Joe Jin <joe.jin@oracle.com>
>>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
>>> ---
>>> Changed since v1:
>>> - change format for "none" as well.
>>>
>>>  monitor/hmp.c | 9 +++++++--
>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/monitor/hmp.c b/monitor/hmp.c
>>> index 15ca047..467fc84 100644
>>> --- a/monitor/hmp.c
>>> +++ b/monitor/hmp.c
>>> @@ -285,10 +285,15 @@ void help_cmd(Monitor *mon, const char *name)
>>>          if (!strcmp(name, "log")) {
>>>              const QEMULogItem *item;
>>>              monitor_printf(mon, "Log items (comma separated):\n");
>>> -            monitor_printf(mon, "%-10s %s\n", "none", "remove all logs");
>>> +            monitor_printf(mon, "%-15s %s\n", "none", "remove all logs");
>>>              for (item = qemu_log_items; item->mask != 0; item++) {
>>> -                monitor_printf(mon, "%-10s %s\n", item->name, item->help);
>>> +                monitor_printf(mon, "%-15s %s\n", item->name, item->help);
>>>              }
>>> +#ifdef CONFIG_TRACE_LOG
>>> +            monitor_printf(mon, "trace:PATTERN   enable trace events\n");
>>> +            monitor_printf(mon, "\nUse \"info trace-events\" to get a list of "
>>> +                                "trace events.\n\n");
>> 
>> Aha: it fixes help to show "log trace:PATTERN".  Was that forgotten in
>> Paolo's commit c84ea00dc2 'log: add "-d trace:PATTERN"'?
>
> I will add the Fixes tag.
>
>> 
>> "info trace-events", hmmm... it shows trace events and their state.
>> "log trace:help" also lists them, less their state, and in opposite
>> order.  Why do we need both?

I guess we have both because we want an HMP command to show the state of
trace events ("info trace-events"), and we want "-d trace" to provide
help.

The latter also lets HMP command "log trace" help, which feels less
important to me, since "info trace-events" exists and is easier to find
and significantly more usable than "log trace:help": it can filter its
output, and unfiltered output is too long to be useful without something
like grep.

Could the two share more code?

Hmm, there seems to be something wrong with "log trace:help": I see
truncated output.  Moreover, output goes to stdout instead of the
monitor.  That's wrong.  Any help you can also emit from the monitor
should be printed with qemu_printf().

> I will print "log trace:help" in the help output.
>
>> What about showing them in alphabetical order?
>
> The order is following how they are defined in the qemu_log_items[] array. To
> re-order them in the array may introduce more conflicts when backporting a
> util/log patch to QEMU old version.
>
> Please let me know if you prefer to re-order. Otherwise, I prefer to avoid that.

I'm talking about the output of "log trace:help", not the output of "log
help".

> Thank you very much for the suggestions!
>
> Dongli Zhang
>
>> 
>>> +#endif
>>>              return;
>>>          }
>>