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

[Dongli Zhang]

Hi Markus,

On 8/31/22 11:47 PM, Markus Armbruster wrote:
> 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?

Thank you very much for the suggestion. I will try if they can share the 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().

Currently the "log trace:help" prints via fprintf() to stdout. I will fix this.

169 void trace_list_events(FILE *f)
170 {
171     TraceEventIter iter;
172     TraceEvent *ev;
173     trace_event_iter_init_all(&iter);
174     while ((ev = trace_event_iter_next(&iter)) != NULL) {
175         fprintf(f, "%s\n", trace_event_get_name(ev));
176     }
177 #ifdef CONFIG_TRACE_DTRACE
178     fprintf(f, "This list of names of trace points may be incomplete "
179                "when using the DTrace/SystemTap backends.\n"
180                "Run 'qemu-trace-stap list %s' to print the full list.\n",
181             g_get_prgname());
182 #endif
183 }

> 
>> 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".

The order is guaranteed by each trace group. To re-order may require some works
with scripts/tracetool when the events from each group/folder are united. I will
have a confirm.

Thank you very much for suggestions!

Dongli Zhang