Re: [PULL 10/11] trace: document how to specify multiple --trace patterns

[Markus Armbruster]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Mon, 1 Feb 2021, Paolo Bonzini wrote:
>> On 01/02/21 17:54, Kevin Wolf wrote:
>>>> How does this option parsing work? Would then multiple patterns
>>>> separated by
>>>> comma as in -trace pattern1,pattern2 also work?
>>> This would be interpreted as an implied "enable" option with a value of
>>> "pattern1,pattern2". I don't think anything splits that string at the
>>> comma, so it would look for a trace event matching that string.
>>
>> Even worse, it would be interpreted as "-trace
>> enable=pattern1,pattern2=on" (and raise a warning since recently).
>
> Not very intuitive... What would -trace
> enable=pattern1,enable=pattern2 do then?

Welcome to the QemuOpts swamp.  Bring your own mosquito net.

The argument of -trace is parsed with QemuOpts.

The option argument is specified in trace/control.c:

    QemuOptsList qemu_trace_opts = {
        .name = "trace",
        .implied_opt_name = "enable",
        .head = QTAILQ_HEAD_INITIALIZER(qemu_trace_opts.head),
        .desc = {
            {
                .name = "enable",
                .type = QEMU_OPT_STRING,
            },
            {
                .name = "events",
                .type = QEMU_OPT_STRING,
            },{
                .name = "file",
                .type = QEMU_OPT_STRING,
            },
            { /* end of list */ }
        },
    };

We generally refer to QemuOptsList by name.  This one's name is "trace".

The non-empty .desc[] enumerates the recognized parameters.
Additionally, special parameter "id" is recognized.

.implied_opt_name enables "omitted first key defaults to implied key"
sugar.  This is what makes "-trace PATTERN" shorthand for -trace
enable=PATTERN", where PATTERN contains neither '=' nor unescaped ','.

The QemuOpts parser parses an option argument string into a QemuOpts,
stores it for later use, and also returns it for immediate use.

Code can do whatever it wants with the stored parameters.  This is a
wellspring of inconsistency and confusion.

Let's look at the code for -trace.  In qemu_init(), we have:

                case QEMU_OPTION_trace:
                    trace_opt_parse(optarg);
                    break;

This calls trace_opt_parse() for every -trace, in order.  @optarg is the
argument string.

    void trace_opt_parse(const char *optarg)
    {
        QemuOpts *opts = qemu_opts_parse_noisily(qemu_find_opts("trace"),
                                                 optarg, true);

qemu_opts_parse_noisily() parses @optarg into a QemuOpts, stores it for
later use, and also returns it for immediate use.

        if (!opts) {
            exit(1);
        }

        if (qemu_opt_get(opts, "enable")) {
            trace_enable_events(qemu_opt_get(opts, "enable"));
        }

Pass the last enable=PATTERN in @optarg to trace_enable_events().

        trace_init_events(qemu_opt_get(opts, "events"));

Pass the the last events=FILENAME to trace_init_events(), which parses
patterns from file FILENAME and passes them to trace_enable_events().

Non-last enable=... ane events=... are silently ignored.

        init_trace_on_startup = true;

Set a flag for trace_init_file().

        qemu_opts_del(opts);

Delete the stored QemuOpts.  We'll get back to this in jiffie.

    }

Later in qemu_init(), we call trace_init_file().  Here it is:

    void trace_init_file(void)
    {
        QemuOpts *opts = qemu_find_opts_singleton("trace");

This gets the first QemuOpts stored in the QemuOptsList named "trace"
without "id".  If there is none, it creates an empty one for us.

Since trace_opt_parse() deletes, this always creates an empty one.

        const char *file = qemu_opt_get(opts, "file");

This is always null.

    #ifdef CONFIG_TRACE_SIMPLE
        st_set_trace_file(file);
        if (init_trace_on_startup) {
            st_set_trace_file_enabled(true);
        }
    #elif defined CONFIG_TRACE_LOG
        /*
         * If both the simple and the log backends are enabled, "--trace file"
         * only applies to the simple backend; use "-D" for the log
         * backend. However we should only override -D if we actually have
         * something to override it with.
         */
        if (file) {
            qemu_set_log_filename(file, &error_fatal);
        }
    #else
        if (file) {
            fprintf(stderr, "error: --trace file=...: "
                    "option not supported by the selected tracing backends\n");
            exit(1);
        }
    #endif
    }

Bug: option parameter "file" has no effect.  I suspect this was broken
in commit 92eecfff32 "trace: remove argument from trace_init_file",
2020-11-11.

And now I'm ready to answer your question:

    -trace enable=pattern1,enable=pattern2

is a confusing way to say

    -trace enable=pattern2

To specify both patterns, use

    -trace enable=pattern1 -trace enable=pattern2

Lovely, isn't it?