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

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

[Dongli Zhang]

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.

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");
+#endif
             return;
         }
 
-- 
1.8.3.1

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

[Markus Armbruster]

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.

> 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"'?

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

What about showing them in alphabetical order?

> +#endif
>              return;
>          }

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

[Dongli Zhang]

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

Thank you very much for the suggestions!

Dongli Zhang

> 
>> +#endif
>>              return;
>>          }
> 

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

[Markus Armbruster]

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;
>>>          }
>> 

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