[v2] trace/simple: Fix unauthorized enable

[PATCH v2] trace/simple: Fix unauthorized enable

Posted by Markus Armbruster 5 years, 5 months ago

st_set_trace_file() accidentally enables tracing.  It's called
unconditionally during startup, which is why QEMU built with the
simple trace backend always writes a trace file "trace-$PID".

This has been broken for quite a while.  I didn't track down the exact
commit.

Fix st_set_trace_file() to restore the state.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
v2: Make st_set_trace_file() less confusing [Stefan]

 trace/simple.h |  2 +-
 trace/simple.c | 20 ++++++++++++++------
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/trace/simple.h b/trace/simple.h
index 5771a0634f..26ccbc8b8a 100644
--- a/trace/simple.h
+++ b/trace/simple.h
@@ -12,7 +12,7 @@
 #define TRACE_SIMPLE_H
 
 void st_print_trace_file_status(void);
-void st_set_trace_file_enabled(bool enable);
+bool st_set_trace_file_enabled(bool enable);
 void st_set_trace_file(const char *file);
 bool st_init(void);
 void st_flush_trace_buffer(void);
diff --git a/trace/simple.c b/trace/simple.c
index fc7106ec49..9cd2ed1fb3 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -302,10 +302,17 @@ static int st_write_event_mapping(void)
     return 0;
 }
 
-void st_set_trace_file_enabled(bool enable)
+/**
+ * Enable / disable tracing, return whether it was enabled.
+ *
+ * @enable: enable if %true, else disable.
+ */
+bool st_set_trace_file_enabled(bool enable)
 {
+    bool was_enabled = trace_fp;
+
     if (enable == !!trace_fp) {
-        return; /* no change */
+        return was_enabled;     /* no change */
     }
 
     /* Halt trace writeout */
@@ -323,14 +330,14 @@ void st_set_trace_file_enabled(bool enable)
 
         trace_fp = fopen(trace_file_name, "wb");
         if (!trace_fp) {
-            return;
+            return was_enabled;
         }
 
         if (fwrite(&header, sizeof header, 1, trace_fp) != 1 ||
             st_write_event_mapping() < 0) {
             fclose(trace_fp);
             trace_fp = NULL;
-            return;
+            return was_enabled;
         }
 
         /* Resume trace writeout */
@@ -340,6 +347,7 @@ void st_set_trace_file_enabled(bool enable)
         fclose(trace_fp);
         trace_fp = NULL;
     }
+    return was_enabled;
 }
 
 /**
@@ -350,7 +358,7 @@ void st_set_trace_file_enabled(bool enable)
  */
 void st_set_trace_file(const char *file)
 {
-    st_set_trace_file_enabled(false);
+    bool saved_enable = st_set_trace_file_enabled(false);
 
     g_free(trace_file_name);
 
@@ -361,7 +369,7 @@ void st_set_trace_file(const char *file)
         trace_file_name = g_strdup_printf("%s", file);
     }
 
-    st_set_trace_file_enabled(true);
+    st_set_trace_file_enabled(saved_enable);
 }
 
 void st_print_trace_file_status(void)
-- 
2.21.3

Re: [PATCH v2] trace/simple: Fix unauthorized enable

Posted by Philippe Mathieu-Daudé 5 years, 5 months ago

On 5/27/20 8:56 AM, Markus Armbruster wrote:
> st_set_trace_file() accidentally enables tracing.  It's called
> unconditionally during startup, which is why QEMU built with the
> simple trace backend always writes a trace file "trace-$PID".
> 
> This has been broken for quite a while.  I didn't track down the exact
> commit.
> 
> Fix st_set_trace_file() to restore the state.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> v2: Make st_set_trace_file() less confusing [Stefan]
> 
>  trace/simple.h |  2 +-
>  trace/simple.c | 20 ++++++++++++++------
>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/trace/simple.h b/trace/simple.h
> index 5771a0634f..26ccbc8b8a 100644
> --- a/trace/simple.h
> +++ b/trace/simple.h
> @@ -12,7 +12,7 @@
>  #define TRACE_SIMPLE_H
>  
>  void st_print_trace_file_status(void);
> -void st_set_trace_file_enabled(bool enable);
> +bool st_set_trace_file_enabled(bool enable);
>  void st_set_trace_file(const char *file);
>  bool st_init(void);
>  void st_flush_trace_buffer(void);
> diff --git a/trace/simple.c b/trace/simple.c
> index fc7106ec49..9cd2ed1fb3 100644
> --- a/trace/simple.c
> +++ b/trace/simple.c
> @@ -302,10 +302,17 @@ static int st_write_event_mapping(void)
>      return 0;
>  }
>  
> -void st_set_trace_file_enabled(bool enable)
> +/**
> + * Enable / disable tracing, return whether it was enabled.
> + *
> + * @enable: enable if %true, else disable.
> + */
> +bool st_set_trace_file_enabled(bool enable)
>  {
> +    bool was_enabled = trace_fp;
> +
>      if (enable == !!trace_fp) {
> -        return; /* no change */
> +        return was_enabled;     /* no change */
>      }
>  
>      /* Halt trace writeout */
> @@ -323,14 +330,14 @@ void st_set_trace_file_enabled(bool enable)
>  
>          trace_fp = fopen(trace_file_name, "wb");
>          if (!trace_fp) {
> -            return;
> +            return was_enabled;
>          }
>  
>          if (fwrite(&header, sizeof header, 1, trace_fp) != 1 ||
>              st_write_event_mapping() < 0) {
>              fclose(trace_fp);
>              trace_fp = NULL;
> -            return;
> +            return was_enabled;
>          }
>  
>          /* Resume trace writeout */
> @@ -340,6 +347,7 @@ void st_set_trace_file_enabled(bool enable)
>          fclose(trace_fp);
>          trace_fp = NULL;
>      }
> +    return was_enabled;
>  }
>  
>  /**
> @@ -350,7 +358,7 @@ void st_set_trace_file_enabled(bool enable)
>   */
>  void st_set_trace_file(const char *file)
>  {
> -    st_set_trace_file_enabled(false);
> +    bool saved_enable = st_set_trace_file_enabled(false);
>  
>      g_free(trace_file_name);
>  
> @@ -361,7 +369,7 @@ void st_set_trace_file(const char *file)
>          trace_file_name = g_strdup_printf("%s", file);
>      }
>  
> -    st_set_trace_file_enabled(true);
> +    st_set_trace_file_enabled(saved_enable);
>  }
>  
>  void st_print_trace_file_status(void)
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2] trace/simple: Fix unauthorized enable

Posted by Stefan Hajnoczi 5 years, 5 months ago

On Wed, May 27, 2020 at 08:56:13AM +0200, Markus Armbruster wrote:
> st_set_trace_file() accidentally enables tracing.  It's called
> unconditionally during startup, which is why QEMU built with the
> simple trace backend always writes a trace file "trace-$PID".
> 
> This has been broken for quite a while.  I didn't track down the exact
> commit.
> 
> Fix st_set_trace_file() to restore the state.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> v2: Make st_set_trace_file() less confusing [Stefan]
> 
>  trace/simple.h |  2 +-
>  trace/simple.c | 20 ++++++++++++++------
>  2 files changed, 15 insertions(+), 7 deletions(-)

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

Stefan