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
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. 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@intel.com>
---
 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));
 		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",
-- 
2.42.0

Ki,

You're lacking a few people from the To/Cc list. Please see KERNEL 
SELFTEST FRAMEWORK entry in MAINTAINERS.

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.

> 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.

> 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@intel.com>
> ---
>  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. 

>  		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.

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@intel.com>
>> ---
>>  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

On Fri, 25 Aug 2023, Maciej Wieczór-Retman wrote:
> 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.

Another approach would be to use syscalls directly (open, write, and 
close to eliminate the buffering entirely. Given schema is already 
written into local variable first, it would be quite straightforward to do 
that conversion.


-- 
 i.

On 2023-08-25 at 11:43:22 +0300, Ilpo Järvinen wrote:
>On Fri, 25 Aug 2023, Maciej Wieczór-Retman wrote:
>> 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.
>
>Another approach would be to use syscalls directly (open, write, and 
>close to eliminate the buffering entirely. Given schema is already 
>written into local variable first, it would be quite straightforward to do 
>that conversion.

Okay, I'll try that then, thanks

-- 
Kind regards
Maciej Wieczór-Retman