[PATCH] trace/simple: Fix unauthorized enable

[PATCH] trace/simple: Fix unauthorized enable

[Markus Armbruster]

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>
---
 trace/simple.h |  2 +-
 trace/simple.c | 13 +++++++------
 2 files changed, 8 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..906391538f 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -302,10 +302,10 @@ static int st_write_event_mapping(void)
     return 0;
 }
 
-void st_set_trace_file_enabled(bool enable)
+bool st_set_trace_file_enabled(bool enable)
 {
     if (enable == !!trace_fp) {
-        return; /* no change */
+        return enable;          /* no change */
     }
 
     /* Halt trace writeout */
@@ -323,14 +323,14 @@ void st_set_trace_file_enabled(bool enable)
 
         trace_fp = fopen(trace_file_name, "wb");
         if (!trace_fp) {
-            return;
+            return !enable;
         }
 
         if (fwrite(&header, sizeof header, 1, trace_fp) != 1 ||
             st_write_event_mapping() < 0) {
             fclose(trace_fp);
             trace_fp = NULL;
-            return;
+            return !enable;
         }
 
         /* Resume trace writeout */
@@ -340,6 +340,7 @@ void st_set_trace_file_enabled(bool enable)
         fclose(trace_fp);
         trace_fp = NULL;
     }
+    return !enable;
 }
 
 /**
@@ -350,7 +351,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 +362,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.1

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

[Stefan Hajnoczi]

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

On Fri, May 15, 2020 at 09:00:21AM +0200, Markus Armbruster wrote:
> diff --git a/trace/simple.c b/trace/simple.c
> index fc7106ec49..906391538f 100644
> --- a/trace/simple.c
> +++ b/trace/simple.c
> @@ -302,10 +302,10 @@ static int st_write_event_mapping(void)
>      return 0;
>  }
>  
> -void st_set_trace_file_enabled(bool enable)
> +bool st_set_trace_file_enabled(bool enable)
>  {
>      if (enable == !!trace_fp) {
> -        return; /* no change */
> +        return enable;          /* no change */
>      }
>  
>      /* Halt trace writeout */
> @@ -323,14 +323,14 @@ void st_set_trace_file_enabled(bool enable)
>  
>          trace_fp = fopen(trace_file_name, "wb");
>          if (!trace_fp) {
> -            return;
> +            return !enable;
>          }
>  
>          if (fwrite(&header, sizeof header, 1, trace_fp) != 1 ||
>              st_write_event_mapping() < 0) {
>              fclose(trace_fp);
>              trace_fp = NULL;
> -            return;
> +            return !enable;
>          }
>  
>          /* Resume trace writeout */
> @@ -340,6 +340,7 @@ void st_set_trace_file_enabled(bool enable)
>          fclose(trace_fp);
>          trace_fp = NULL;
>      }
> +    return !enable;
>  }

The meaning of the return value confuses me. Is it the previous value
(even when the function fails)?  Please document the meaning.

The code might be easier to understand like this:

  bool st_set_trace_file_enabled(bool enable)
  {
      bool was_enabled = trace_fp;

      if (enable == was_enabled) {
          return was_enabled;      /* no change */
      }

      ...

      return was_enabled;
  }

Now it is not necessary to remember that !enable is the previous value
because we would have already returned if the value was unchanged.

Stefan

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

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

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Fri, May 15, 2020 at 09:00:21AM +0200, Markus Armbruster wrote:
>> diff --git a/trace/simple.c b/trace/simple.c
>> index fc7106ec49..906391538f 100644
>> --- a/trace/simple.c
>> +++ b/trace/simple.c
>> @@ -302,10 +302,10 @@ static int st_write_event_mapping(void)
>>      return 0;
>>  }
>>  
>> -void st_set_trace_file_enabled(bool enable)
>> +bool st_set_trace_file_enabled(bool enable)
>>  {
>>      if (enable == !!trace_fp) {
>> -        return; /* no change */
>> +        return enable;          /* no change */
>>      }
>>  
>>      /* Halt trace writeout */
>> @@ -323,14 +323,14 @@ void st_set_trace_file_enabled(bool enable)
>>  
>>          trace_fp = fopen(trace_file_name, "wb");
>>          if (!trace_fp) {
>> -            return;
>> +            return !enable;
>>          }
>>  
>>          if (fwrite(&header, sizeof header, 1, trace_fp) != 1 ||
>>              st_write_event_mapping() < 0) {
>>              fclose(trace_fp);
>>              trace_fp = NULL;
>> -            return;
>> +            return !enable;
>>          }
>>  
>>          /* Resume trace writeout */
>> @@ -340,6 +340,7 @@ void st_set_trace_file_enabled(bool enable)
>>          fclose(trace_fp);
>>          trace_fp = NULL;
>>      }
>> +    return !enable;
>>  }
>
> The meaning of the return value confuses me. Is it the previous value
> (even when the function fails)?  Please document the meaning.
>
> The code might be easier to understand like this:
>
>   bool st_set_trace_file_enabled(bool enable)
>   {
>       bool was_enabled = trace_fp;
>
>       if (enable == was_enabled) {
>           return was_enabled;      /* no change */
>       }
>
>       ...
>
>       return was_enabled;
>   }
>
> Now it is not necessary to remember that !enable is the previous value
> because we would have already returned if the value was unchanged.

I'll send v2.  Thanks!