Stefan Hajnoczi <stefa...@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!
