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

[Markus Armbruster]

Stefan Hajnoczi <address@hidden> 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!