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

[Stefan Hajnoczi]

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

signature.asc
Description: PGP signature