| Message ID | 20220829170316.3053-1-dongli.zhang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/1] monitor/hmp: print trace as option in help for log command | expand |
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; > }
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; >> } >
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; >>> } >>
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
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; }
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(-)