It is possible to repeat the --trace option to specify multiple
patterns. This may be preferrable to users who do not want to create a
file with a list of patterns.

Suggested-by: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-id: 20210112165859.225534-2-stefanha@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 docs/devel/tracing.rst | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/docs/devel/tracing.rst b/docs/devel/tracing.rst
index af395e957d..e8f9b82c5e 100644
--- a/docs/devel/tracing.rst
+++ b/docs/devel/tracing.rst
@@ -22,10 +22,15 @@ events::
 This output comes from the "log" trace backend that is enabled by default when
 ``./configure --enable-trace-backends=BACKENDS`` was not explicitly specified.
 
-More than one trace event pattern can be specified by providing a file
-instead::
+Multiple patterns can be specified by repeating the ``--trace`` option::
+
+    $ qemu --trace "kvm_*" --trace "virtio_*" ...
+
+When patterns are used frequently it is more convenient to store them in a
+file to avoid long command-line options::
 
     $ echo "memory_region_ops_*" >/tmp/events
+    $ echo "kvm_*" >>/tmp/events
     $ qemu --trace events=/tmp/events ...
 
 Trace events
-- 
2.29.2

On Mon, 1 Feb 2021, Stefan Hajnoczi wrote:
> It is possible to repeat the --trace option to specify multiple
> patterns. This may be preferrable to users who do not want to create a
> file with a list of patterns.
>
> Suggested-by: BALATON Zoltan <balaton@eik.bme.hu>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Message-id: 20210112165859.225534-2-stefanha@redhat.com
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> docs/devel/tracing.rst | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/docs/devel/tracing.rst b/docs/devel/tracing.rst
> index af395e957d..e8f9b82c5e 100644
> --- a/docs/devel/tracing.rst
> +++ b/docs/devel/tracing.rst
> @@ -22,10 +22,15 @@ events::
> This output comes from the "log" trace backend that is enabled by default when
> ``./configure --enable-trace-backends=BACKENDS`` was not explicitly specified.
> 
> -More than one trace event pattern can be specified by providing a file
> -instead::
> +Multiple patterns can be specified by repeating the ``--trace`` option::
> +
> +    $ qemu --trace "kvm_*" --trace "virtio_*" ...

Does that actually work? I've always used -trace enable="pattern1" -trace 
enable="pattern2" Not sure if omitting enable= is the same.

Regards,
BALATON Zoltan

> +
> +When patterns are used frequently it is more convenient to store them in a
> +file to avoid long command-line options::
>
>     $ echo "memory_region_ops_*" >/tmp/events
> +    $ echo "kvm_*" >>/tmp/events
>     $ qemu --trace events=/tmp/events ...
> 
> Trace events
> -- 
> 2.29.2

Am 01.02.2021 um 17:05 hat BALATON Zoltan geschrieben:
> On Mon, 1 Feb 2021, Stefan Hajnoczi wrote:
> > It is possible to repeat the --trace option to specify multiple
> > patterns. This may be preferrable to users who do not want to create a
> > file with a list of patterns.
> > 
> > Suggested-by: BALATON Zoltan <balaton@eik.bme.hu>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Message-id: 20210112165859.225534-2-stefanha@redhat.com
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > docs/devel/tracing.rst | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/docs/devel/tracing.rst b/docs/devel/tracing.rst
> > index af395e957d..e8f9b82c5e 100644
> > --- a/docs/devel/tracing.rst
> > +++ b/docs/devel/tracing.rst
> > @@ -22,10 +22,15 @@ events::
> > This output comes from the "log" trace backend that is enabled by default when
> > ``./configure --enable-trace-backends=BACKENDS`` was not explicitly specified.
> > 
> > -More than one trace event pattern can be specified by providing a file
> > -instead::
> > +Multiple patterns can be specified by repeating the ``--trace`` option::
> > +
> > +    $ qemu --trace "kvm_*" --trace "virtio_*" ...
> 
> Does that actually work? I've always used -trace enable="pattern1" -trace
> enable="pattern2" Not sure if omitting enable= is the same.

qemu_trace_opts has .implied_opt_name = "enable", so without having
tested it, I assume this works.

Kevin

On 2/1/21 5:13 PM, Kevin Wolf wrote:
> Am 01.02.2021 um 17:05 hat BALATON Zoltan geschrieben:
>> On Mon, 1 Feb 2021, Stefan Hajnoczi wrote:
>>> It is possible to repeat the --trace option to specify multiple
>>> patterns. This may be preferrable to users who do not want to create a
>>> file with a list of patterns.
>>>
>>> Suggested-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> Message-id: 20210112165859.225534-2-stefanha@redhat.com
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>> docs/devel/tracing.rst | 9 +++++++--
>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/docs/devel/tracing.rst b/docs/devel/tracing.rst
>>> index af395e957d..e8f9b82c5e 100644
>>> --- a/docs/devel/tracing.rst
>>> +++ b/docs/devel/tracing.rst
>>> @@ -22,10 +22,15 @@ events::
>>> This output comes from the "log" trace backend that is enabled by default when
>>> ``./configure --enable-trace-backends=BACKENDS`` was not explicitly specified.
>>>
>>> -More than one trace event pattern can be specified by providing a file
>>> -instead::
>>> +Multiple patterns can be specified by repeating the ``--trace`` option::
>>> +
>>> +    $ qemu --trace "kvm_*" --trace "virtio_*" ...
>>
>> Does that actually work? I've always used -trace enable="pattern1" -trace
>> enable="pattern2" Not sure if omitting enable= is the same.
> 
> qemu_trace_opts has .implied_opt_name = "enable", so without having
> tested it, I assume this works.

I use -trace \*pci\* -trace memory\*, and Kevin said -trace and --trace
are the same.

On Mon, 1 Feb 2021, Kevin Wolf wrote:
> Am 01.02.2021 um 17:05 hat BALATON Zoltan geschrieben:
>> On Mon, 1 Feb 2021, Stefan Hajnoczi wrote:
>>> It is possible to repeat the --trace option to specify multiple
>>> patterns. This may be preferrable to users who do not want to create a
>>> file with a list of patterns.
>>>
>>> Suggested-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> Message-id: 20210112165859.225534-2-stefanha@redhat.com
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>> docs/devel/tracing.rst | 9 +++++++--
>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/docs/devel/tracing.rst b/docs/devel/tracing.rst
>>> index af395e957d..e8f9b82c5e 100644
>>> --- a/docs/devel/tracing.rst
>>> +++ b/docs/devel/tracing.rst
>>> @@ -22,10 +22,15 @@ events::
>>> This output comes from the "log" trace backend that is enabled by default when
>>> ``./configure --enable-trace-backends=BACKENDS`` was not explicitly specified.
>>>
>>> -More than one trace event pattern can be specified by providing a file
>>> -instead::
>>> +Multiple patterns can be specified by repeating the ``--trace`` option::
>>> +
>>> +    $ qemu --trace "kvm_*" --trace "virtio_*" ...
>>
>> Does that actually work? I've always used -trace enable="pattern1" -trace
>> enable="pattern2" Not sure if omitting enable= is the same.
>
> qemu_trace_opts has .implied_opt_name = "enable", so without having
> tested it, I assume this works.

How does this option parsing work? Would then multiple patterns separated 
by comma as in -trace pattern1,pattern2 also work? (Although quoting * in 
that may be a challenge don't know if it should be "a*,b*" "a*","b*" or 
a\*,b\* or any of these would be OK.) I've just found one way by repeating 
-trace options that worked and then used that without ever trying to 
explore other ways but if we're about to document it maybe somebody who 
actually knows how it works could tell what is the best way.

Regards,
BALATON Zoltan

Am 01.02.2021 um 17:29 hat BALATON Zoltan geschrieben:
> On Mon, 1 Feb 2021, Kevin Wolf wrote:
> > Am 01.02.2021 um 17:05 hat BALATON Zoltan geschrieben:
> > > On Mon, 1 Feb 2021, Stefan Hajnoczi wrote:
> > > > It is possible to repeat the --trace option to specify multiple
> > > > patterns. This may be preferrable to users who do not want to create a
> > > > file with a list of patterns.
> > > > 
> > > > Suggested-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > > Message-id: 20210112165859.225534-2-stefanha@redhat.com
> > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > ---
> > > > docs/devel/tracing.rst | 9 +++++++--
> > > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/docs/devel/tracing.rst b/docs/devel/tracing.rst
> > > > index af395e957d..e8f9b82c5e 100644
> > > > --- a/docs/devel/tracing.rst
> > > > +++ b/docs/devel/tracing.rst
> > > > @@ -22,10 +22,15 @@ events::
> > > > This output comes from the "log" trace backend that is enabled by default when
> > > > ``./configure --enable-trace-backends=BACKENDS`` was not explicitly specified.
> > > > 
> > > > -More than one trace event pattern can be specified by providing a file
> > > > -instead::
> > > > +Multiple patterns can be specified by repeating the ``--trace`` option::
> > > > +
> > > > +    $ qemu --trace "kvm_*" --trace "virtio_*" ...
> > > 
> > > Does that actually work? I've always used -trace enable="pattern1" -trace
> > > enable="pattern2" Not sure if omitting enable= is the same.
> > 
> > qemu_trace_opts has .implied_opt_name = "enable", so without having
> > tested it, I assume this works.
> 
> 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.

Kevin

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

Paolo

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?

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?

On 02/02/21 13:41, Markus Armbruster wrote:
> 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.

Indeed, and I'll fix it.

Paolo

On Tue, 2 Feb 2021, Markus Armbruster wrote:
> 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?

OK, interesting. The conclusion is then that the doc change in this patch 
_is_ the simplest way to enable multiple traces as only repeating the 
-trace option works and enable= is optional in this case. Thank you for 
the detailed explanation.

Regards,
BALATON Zoltan

On Mon, Feb 01, 2021 at 06:25:33PM +0100, 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).

Maybe we're trying to solve the problem at the wrong level.

The pattern is currently matched using the GLib glob matching APIs.

If we switched to use the GLib regex matching APIs, then we don't need
to repeat the args at all. We could just use regex syntax:

  -trace 'enable=(kvm|virtio)*'

It is a little tedious to have to quote the args to avoid shell
expansion, but as a tradeoff you get much stronger ability todo
complex matches to filter out irrelevant cruft.

If we want to maintain back compat for glob syntax, then we should
support both in parallel and accept a different parameter name
for the regex style.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Mon, 1 Feb 2021, Daniel P. Berrangé wrote:
> On Mon, Feb 01, 2021 at 06:25:33PM +0100, 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).
>
> Maybe we're trying to solve the problem at the wrong level.

There's no problem to solve, just trying to understand better what are the 
valid options. It's already possible to enable multiple patterns with 
either events=file or repeating -trace options (with or without enable=) 
so that's already sufficient, I was just curious what other options are 
there and if there's a simpler way that we could document. If not, using 
the current ways that are now documented is OK I think.

> The pattern is currently matched using the GLib glob matching APIs.
>
> If we switched to use the GLib regex matching APIs, then we don't need
> to repeat the args at all. We could just use regex syntax:
>
>  -trace 'enable=(kvm|virtio)*'
>
> It is a little tedious to have to quote the args to avoid shell
> expansion, but as a tradeoff you get much stronger ability todo
> complex matches to filter out irrelevant cruft.

I guess we have enough expressiveness with current pattern matching and 
globs are more easily understood by mere users so regex may not be needed 
here.

> If we want to maintain back compat for glob syntax, then we should
> support both in parallel and accept a different parameter name
> for the regex style.

That would be (even more) confusing so better to just stay with globs.

Regards,
BALATON Zoltan

On Mon, Feb 01, 2021 at 07:13:27PM +0100, BALATON Zoltan wrote:
> On Mon, 1 Feb 2021, Daniel P. Berrangé wrote:
> > On Mon, Feb 01, 2021 at 06:25:33PM +0100, 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).
> > 
> > Maybe we're trying to solve the problem at the wrong level.
> 
> There's no problem to solve, just trying to understand better what are the
> valid options. It's already possible to enable multiple patterns with either
> events=file or repeating -trace options (with or without enable=) so that's
> already sufficient, I was just curious what other options are there and if
> there's a simpler way that we could document. If not, using the current ways
> that are now documented is OK I think.

The enable=PATTERN syntax is very limited. Repeating the --trace option
is currently the only way to enter multiple patterns on the
command-line.

Stefan