Re: [PATCH v3 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 9/17/22 2:44 PM, Philippe Mathieu-Daudé via wrote:
>> Hi Markus,
>> 
>> On 2/9/22 14:24, 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, the options of "trace:PATTERN" are only printed by
>>>> "qemu-system-x86_64 -d help", but missing in hmp "help log" command.
>>>>
>>>> Fixes: c84ea00dc2 ("log: add "-d trace:PATTERN"")
>>>> 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.
>>>> Changed since v2:
>>>> - use "log trace:help" in help message.
>>>> - add more clarification in commit message.
>>>> - add 'Fixes' tag.
>>>> ---
>>>>   monitor/hmp.c | 9 +++++++--
>>>>   1 file changed, 7 insertions(+), 2 deletions(-)
>> 
>>> Not this patch's fault:
>>>
>>> 1. "-d help" terminates with exit status 1, "-d trace:help" with 0.  The
>>>     former is wrong.
>
> May I assume it is expected to have exit status 1 when "-d help"?

Non-zero exit status means error.  Asking for and receiving help is not
an error.  Therefore, "-d help" should exit with status 0, just like
"-help", "-device help", "-machine help", ...

> According to the output of "-d", there is even not a "help" option, but only a
> "-d trace:help" option. That is, "-d help" is not officially supported.

It *is* documented:

    $ qemu-system-x86_64 -help | grep -- '^-d '
    -d item1,...    enable logging of specified items (use '-d help' for a list of log items)

> The below example use "-d hellworld" but not "help".
>
> # qemu-system-x86_64 -d helloworld
> Log items (comma separated):
> out_asm         show generated host assembly code for each compiled TB
> in_asm          show target assembly code for each compiled TB
> op              show micro ops for each compiled TB
> op_opt          show micro ops after optimization
> op_ind          show micro ops before indirect lowering
> int             show interrupts/exceptions in short format
> exec            show trace before each executed TB (lots of logs)
> cpu             show CPU registers before entering a TB (lots of logs)
> fpu             include FPU registers in the 'cpu' logging
> mmu             log MMU-related activities
> pcall           x86 only: show protected mode far calls/returns/exceptions
> cpu_reset       show CPU state before CPU resets
> unimp           log unimplemented functionality
> guest_errors    log when the guest OS does something invalid (eg accessing a
> non-existent register)
> page            dump pages at beginning of user mode emulation
> nochain         do not chain compiled TBs so that "exec" and "cpu" show
> complete traces
> plugin          output from TCG plugins
>
> strace          log every user-mode syscall, its input, and its result
> tid             open a separate log file per thread; filename must contain '%d'
> trace:PATTERN   enable trace events
>
> Use "-d trace:help" to get a list of trace events.
>
>
> According to the source code, the qemu_str_to_log_mask() expects either log
> items or "trace". For any other inputs (e.g., "help" or "helloworld"),
> qemu_str_to_log_mask() returns 0 (no bit set in the mask).

You're right.

>                                                            That indicates the
> input (e.g., "help") is not an expected input.

No, it indicates laziness :)

> Therefore, can I assume this is not a bug? I do not think something like below
> is very helpful.
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 263f029a8e..54c8e624bf 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2389,6 +2389,8 @@ static void qemu_process_early_options(void)
>              mask = qemu_str_to_log_mask(log_mask);
>              if (!mask) {
>                  qemu_print_log_usage(stdout);
> +                if (g_str_equal(log_mask, "help"))
> +                    exit(0)
>                  exit(1);
>              }
>          }

Let's make "-d help" print help to stdout and terminate successfully,
and "-d crap" report an error and terminate unsuccessfully.  Just like
other options, such as -device and -machine.

> Thank you very much!

You're welcome!