[PATCH 0/2] trace: fix "-trace file=...."

[PATCH 0/2] trace: fix "-trace file=...."

[Paolo Bonzini]

This series contains two fixes for "-trace file=...".  The first applies
to all backends, while the second only applies to the log backend.

Paolo Bonzini (2):
  trace: fix "-trace file=..."
  trace: skip qemu_set_log_filename if no "-D" option was passed

 softmmu/vl.c    |  4 +++-
 trace/control.c | 13 +++++++------
 2 files changed, 10 insertions(+), 7 deletions(-)

-- 
2.29.2

[PATCH 1/2] trace: fix "-trace file=..."

[Paolo Bonzini]

Because trace_opt_parse always deletes the options it has parsed,
trace_init_file's call to qemu_find_opts_singleton always
creates an empty -trace option group.  Therefore, the subsequent
qemu_opt_get(opts, "file") always returns NULL.

To fix this, save the last "-trace file=..." option in a global
variable and use it later in trace_init_file.

This is similar to what was done before commit 92eecfff32 ("trace:
remove argument from trace_init_file", 2020-11-11), except contained
within trace/control.c and without memory leaks.

Fixes: 92eecfff32 ("trace: remove argument from trace_init_file", 2020-11-11)
Cc: stefanha@redhat.com
Reported-by: armbru@redhat.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 trace/control.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/trace/control.c b/trace/control.c
index cd04dd4e0c..4be38e1af2 100644
--- a/trace/control.c
+++ b/trace/control.c
@@ -40,6 +40,7 @@ static size_t nevent_groups;
 static uint32_t next_id;
 static uint32_t next_vcpu_id;
 static bool init_trace_on_startup;
+static char *trace_opts_file;
 
 QemuOptsList qemu_trace_opts = {
     .name = "trace",
@@ -224,10 +225,8 @@ static void trace_init_events(const char *fname)
 
 void trace_init_file(void)
 {
-    QemuOpts *opts = qemu_find_opts_singleton("trace");
-    const char *file = qemu_opt_get(opts, "file");
 #ifdef CONFIG_TRACE_SIMPLE
-    st_set_trace_file(file);
+    st_set_trace_file(trace_opts_file);
     if (init_trace_on_startup) {
         st_set_trace_file_enabled(true);
     }
@@ -238,11 +237,11 @@ void trace_init_file(void)
      * 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);
+    if (trace_opts_file) {
+        qemu_set_log_filename(trace_opts_file, &error_fatal);
     }
 #else
-    if (file) {
+    if (trace_opts_file) {
         fprintf(stderr, "error: --trace file=...: "
                 "option not supported by the selected tracing backends\n");
         exit(1);
@@ -303,6 +302,8 @@ void trace_opt_parse(const char *optarg)
     }
     trace_init_events(qemu_opt_get(opts, "events"));
     init_trace_on_startup = true;
+    g_free(trace_opts_file);
+    trace_opts_file = g_strdup(qemu_opt_get(opts, "file"));
     qemu_opts_del(opts);
 }
 
-- 
2.29.2

[PATCH 2/2] trace: skip qemu_set_log_filename if no "-D" option was passed

[Paolo Bonzini]

When the "simple" backend is not active but the "log" backend is,
both "-trace file=" and "-D" will result in a call to
qemu_set_log_filename.  Unfortunately, QEMU was also calling
qemu_set_log_filename if "-D" was not passed, so the "-trace
file=" option had no effect and the tracepoints went back to
stderr.

Fortunately we can just skip qemu_set_log_filename in that case,
because the log backend will initialize itself just fine as soon
as qemu_set_log is called, also in qemu_process_early_options.

Cc: stefanha@redhat.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 softmmu/vl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index b219ce1f35..e67f91dd37 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2367,7 +2367,9 @@ static void qemu_process_early_options(void)
     trace_init_file();
 
     /* Open the logfile at this point and set the log mask if necessary.  */
-    qemu_set_log_filename(log_file, &error_fatal);
+    if (log_file) {
+        qemu_set_log_filename(log_file, &error_fatal);
+    }
     if (log_mask) {
         int mask;
         mask = qemu_str_to_log_mask(log_mask);
-- 
2.29.2

Re: [PATCH 1/2] trace: fix "-trace file=..."

[Eric Blake]

On 2/9/21 8:57 AM, Paolo Bonzini wrote:
> Because trace_opt_parse always deletes the options it has parsed,
> trace_init_file's call to qemu_find_opts_singleton always
> creates an empty -trace option group.  Therefore, the subsequent
> qemu_opt_get(opts, "file") always returns NULL.
> 
> To fix this, save the last "-trace file=..." option in a global
> variable and use it later in trace_init_file.
> 
> This is similar to what was done before commit 92eecfff32 ("trace:
> remove argument from trace_init_file", 2020-11-11), except contained
> within trace/control.c and without memory leaks.
> 
> Fixes: 92eecfff32 ("trace: remove argument from trace_init_file", 2020-11-11)
> Cc: stefanha@redhat.com
> Reported-by: armbru@redhat.com
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  trace/control.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/2] trace: skip qemu_set_log_filename if no "-D" option was passed

[Eric Blake]

On 2/9/21 8:57 AM, Paolo Bonzini wrote:
> When the "simple" backend is not active but the "log" backend is,
> both "-trace file=" and "-D" will result in a call to
> qemu_set_log_filename.  Unfortunately, QEMU was also calling
> qemu_set_log_filename if "-D" was not passed, so the "-trace
> file=" option had no effect and the tracepoints went back to
> stderr.
> 
> Fortunately we can just skip qemu_set_log_filename in that case,
> because the log backend will initialize itself just fine as soon
> as qemu_set_log is called, also in qemu_process_early_options.
> 
> Cc: stefanha@redhat.com
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  softmmu/vl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index b219ce1f35..e67f91dd37 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2367,7 +2367,9 @@ static void qemu_process_early_options(void)
>      trace_init_file();
>  
>      /* Open the logfile at this point and set the log mask if necessary.  */
> -    qemu_set_log_filename(log_file, &error_fatal);
> +    if (log_file) {
> +        qemu_set_log_filename(log_file, &error_fatal);
> +    }
>      if (log_mask) {
>          int mask;
>          mask = qemu_str_to_log_mask(log_mask);
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 0/2] trace: fix "-trace file=...."

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 589 bytes --]

On Tue, Feb 09, 2021 at 03:57:57PM +0100, Paolo Bonzini wrote:
> This series contains two fixes for "-trace file=...".  The first applies
> to all backends, while the second only applies to the log backend.
> 
> Paolo Bonzini (2):
>   trace: fix "-trace file=..."
>   trace: skip qemu_set_log_filename if no "-D" option was passed
> 
>  softmmu/vl.c    |  4 +++-
>  trace/control.c | 13 +++++++------
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 
> -- 
> 2.29.2
> 

Thanks, applied to my tracing tree:
https://gitlab.com/stefanha/qemu/commits/tracing

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]