Re: [PATCH 1/3] selftests/resctrl: Fix schemata write error check

[Maciej Wieczór-Retman]

Hi,

On 2023-08-24 at 15:52:05 +0300, Ilpo Järvinen wrote:
>Ki,
>
>You're lacking a few people from the To/Cc list. Please see KERNEL 
>SELFTEST FRAMEWORK entry in MAINTAINERS.

Thank you, I thought I checked the MAINTAINERS file well enough. I'll
add them in the next version

>On Thu, 24 Aug 2023, Wieczor-Retman, Maciej wrote:
>
>> Writing bitmasks to the schemata can fail when the bitmask doesn't
>> adhere to some constraints defined by what a particular CPU supports.
>> Some example of constraints are max length or being having contiguous
>
>"being having" is not good English.

Thanks, I'll change it

>> bits. The driver should properly return errors when any rule concerning
>> bitmask format is broken.
>> 
>> Resctrl FS returns error codes from fprintf() only when fclose() is
>> called.
>
>I wonder if this is actually related to libc doing buffering between 
>fprintf() and the actual write() syscall.

I started looking and apparently in the manpages for fclose [1] it says
it uses fflush() to flush any buffered data in the stream. So that would
probably confirm that it does buffering there.

In this case is there a situation when the fprintf() before fclose()
would report an error? I'm thinking if there is a point to keep error
checking after both function calls or just fclose(). 

Or would putting additional fflush() after fprintf() make some sense?
To have separate error checks for both function calls.

>> Current error checking scheme allows invalid bitmasks to be
>> written into schemata file and the selftest doesn't notice because the
>> fclose() error code isn't checked.
>> 
>> Add error check to the fclose() call.
>> 
>> Add perror() just after fprintf so a proper error message can be seen.
>> 
>> Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@xxxxxxxxx>
>> ---
>>  tools/testing/selftests/resctrl/resctrlfs.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
>> index bd36ee206602..a6d0b632cbc6 100644
>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>> @@ -532,13 +532,17 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
>>  	}
>>  
>>  	if (fprintf(fp, "%s\n", schema) < 0) {
>> -		sprintf(reason, "Failed to write schemata in control group");
>> +		sprintf(reason, "fprintf() failed with error : %s",
>> +			strerror(errno));
>
>These should use snprintf() to make sure the buffer does not overflow. 

Sure, I'll change it.

>>  		fclose(fp);
>>  		ret = -1;
>>  
>>  		goto out;
>>  	}
>> -	fclose(fp);
>> +	ret = fclose(fp);
>> +	if (ret)
>> +		sprintf(reason, "Failed to write schemata in control group : %s",
>> +			strerror(errno));
>>  
>>  out:
>>  	ksft_print_msg("Write schema \"%s\" to resctrl FS%s%s\n",
>> 
>
>-- 
> i.
>

[1] https://man7.org/linux/man-pages/man3/fclose.3.html

-- 
Kind regards
Maciej Wieczór-Retman