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

Substitute fopen(), flose() and fprintf() with open(), close() and
write() to avoid error code buffering between fprintf() and fclose().

Add newline to the end of the schema string so it satisfies rdt
schemata writing requirements.

Remove newline character from the schemat string after writing it to
the schemata file so it prints correctly before function return.

Pass the string generated with strerror() to the "reason" buffer so
the error message is more verbose. Extend "reason" buffer so it can hold
longer messages.

Changelog v2:
- Rewrite patch message.
- Double "reason" buffer size to fit longer error explanation.
- Redo file interactions with syscalls instead of stdio functions.

Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
---
 tools/testing/selftests/resctrl/resctrlfs.c | 24 +++++++++++----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index bd36ee206602..0f9644e5a25e 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -488,9 +488,8 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
  */
 int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
 {
-	char controlgroup[1024], schema[1024], reason[64];
-	int resource_id, ret = 0;
-	FILE *fp;
+	char controlgroup[1024], schema[1024], reason[128];
+	int resource_id, fp, ret = 0;
 
 	if (strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) &&
 	    strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) &&
@@ -518,27 +517,30 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
 
 	if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) ||
 	    !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
-		sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=', schemata);
+		snprintf(schema, sizeof(schema), "%s%d%c%s\n", "L3:",
+			 resource_id, '=', schemata);
 	if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) ||
 	    !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)))
-		sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=', schemata);
+		snprintf(schema, sizeof(schema), "%s%d%c%s\n", "MB:",
+			 resource_id, '=', schemata);
 
-	fp = fopen(controlgroup, "w");
+	fp = open(controlgroup, O_WRONLY);
 	if (!fp) {
 		sprintf(reason, "Failed to open control group");
 		ret = -1;
 
 		goto out;
 	}
-
-	if (fprintf(fp, "%s\n", schema) < 0) {
-		sprintf(reason, "Failed to write schemata in control group");
-		fclose(fp);
+	if (write(fp, schema, strlen(schema)) < 0) {
+		snprintf(reason, sizeof(reason),
+			 "write() failed : %s", strerror(errno));
+		close(fp);
 		ret = -1;
 
 		goto out;
 	}
-	fclose(fp);
+	close(fp);
+	schema[strcspn(schema, "\n")] = 0;
 
 out:
 	ksft_print_msg("Write schema \"%s\" to resctrl FS%s%s\n",
-- 
2.42.0

Hi Maciej,

On 8/28/2023 2:56 AM, 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 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.
> 
> Substitute fopen(), flose() and fprintf() with open(), close() and
> write() to avoid error code buffering between fprintf() and fclose().
> 
> Add newline to the end of the schema string so it satisfies rdt
> schemata writing requirements.

I am not sure how to interpret the above because existing code already
adds a newline to the end of the schema when the buffer is written to
the schemata file. Also please use "resctrl schemata" since RDT is
Intel specific and does not use schemata terminology.

> 
> Remove newline character from the schemat string after writing it to
> the schemata file so it prints correctly before function return.

schemat -> "schema" or "schemata"?

> Pass the string generated with strerror() to the "reason" buffer so
> the error message is more verbose. Extend "reason" buffer so it can hold
> longer messages.
> 
> Changelog v2:
> - Rewrite patch message.
> - Double "reason" buffer size to fit longer error explanation.
> - Redo file interactions with syscalls instead of stdio functions.
> 

Please place the above "Changelog v2" snippet below the "---" lines below.
This is text that should not end up in the kernel log.

> Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
> ---

(list of changes should go here)

Reinette



Reinette

Hi,
thanks for the comments!

On 2023-08-30 at 13:49:18 -0700, Reinette Chatre wrote:
>Hi Maciej,
>
>On 8/28/2023 2:56 AM, 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 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.
>> 
>> Substitute fopen(), flose() and fprintf() with open(), close() and
>> write() to avoid error code buffering between fprintf() and fclose().
>> 
>> Add newline to the end of the schema string so it satisfies rdt
>> schemata writing requirements.
>
>I am not sure how to interpret the above because existing code already
>adds a newline to the end of the schema when the buffer is written to

Okay, true. I meant I moved it a few lines back but there is no real
change there. I'll just remove this paragraph.

>the schemata file. Also please use "resctrl schemata" since RDT is
>Intel specific and does not use schemata terminology.

Thank you, I'll change it.

>> 
>> Remove newline character from the schemat string after writing it to
>> the schemata file so it prints correctly before function return.
>
>schemat -> "schema" or "schemata"?

I'll stick with schema as that's the variable name, thanks for finding
this typo.

>> Pass the string generated with strerror() to the "reason" buffer so
>> the error message is more verbose. Extend "reason" buffer so it can hold
>> longer messages.
>> 
>> Changelog v2:
>> - Rewrite patch message.
>> - Double "reason" buffer size to fit longer error explanation.
>> - Redo file interactions with syscalls instead of stdio functions.
>> 
>
>Please place the above "Changelog v2" snippet below the "---" lines below.
>This is text that should not end up in the kernel log.

Yes, I realized I made this mistake a few hours after sending the patch.
I'll correct it and make sure to double check it before sending next time.

>> Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
>> ---
>
>(list of changes should go here)
>
>Reinette
>

-- 
Kind regards
Maciej Wieczór-Retman

On Mon, 28 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 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.
> 
> Substitute fopen(), flose() and fprintf() with open(), close() and
> write() to avoid error code buffering between fprintf() and fclose().
> 
> Add newline to the end of the schema string so it satisfies rdt
> schemata writing requirements.
> 
> Remove newline character from the schemat string after writing it to
> the schemata file so it prints correctly before function return.
> 
> Pass the string generated with strerror() to the "reason" buffer so
> the error message is more verbose. Extend "reason" buffer so it can hold
> longer messages.
> 
> Changelog v2:
> - Rewrite patch message.
> - Double "reason" buffer size to fit longer error explanation.
> - Redo file interactions with syscalls instead of stdio functions.
> 
> Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
> ---
>  tools/testing/selftests/resctrl/resctrlfs.c | 24 +++++++++++----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index bd36ee206602..0f9644e5a25e 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -488,9 +488,8 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
>   */
>  int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
>  {
> -	char controlgroup[1024], schema[1024], reason[64];
> -	int resource_id, ret = 0;
> -	FILE *fp;
> +	char controlgroup[1024], schema[1024], reason[128];
> +	int resource_id, fp, ret = 0;

fp -> fd to follow the usual convention.

>  
>  	if (strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) &&
>  	    strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) &&
> @@ -518,27 +517,30 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
>  
>  	if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) ||
>  	    !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
> -		sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=', schemata);
> +		snprintf(schema, sizeof(schema), "%s%d%c%s\n", "L3:",
> +			 resource_id, '=', schemata);
>  	if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) ||
>  	    !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)))
> -		sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=', schemata);
> +		snprintf(schema, sizeof(schema), "%s%d%c%s\n", "MB:",
> +			 resource_id, '=', schemata);
>  
> -	fp = fopen(controlgroup, "w");
> +	fp = open(controlgroup, O_WRONLY);
>  	if (!fp) {
>  		sprintf(reason, "Failed to open control group");
>  		ret = -1;
>  
>  		goto out;
>  	}
> -
> -	if (fprintf(fp, "%s\n", schema) < 0) {
> -		sprintf(reason, "Failed to write schemata in control group");
> -		fclose(fp);
> +	if (write(fp, schema, strlen(schema)) < 0) {

snprintf() returns you the length, just store the return value and there's 
no need to calculate it here.

> +		snprintf(reason, sizeof(reason),
> +			 "write() failed : %s", strerror(errno));
> +		close(fp);
>  		ret = -1;
>  
>  		goto out;
>  	}
> -	fclose(fp);
> +	close(fp);
> +	schema[strcspn(schema, "\n")] = 0;

Here too you can use the known length/index instead of strcspr().


-- 
 i.

On 2023-08-28 at 13:43:05 +0300, Ilpo Järvinen wrote:
>On Mon, 28 Aug 2023, Wieczor-Retman, Maciej wrote:
>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
>> index bd36ee206602..0f9644e5a25e 100644
>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>> @@ -488,9 +488,8 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
>>   */
>>  int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
>>  {
>> -	char controlgroup[1024], schema[1024], reason[64];
>> -	int resource_id, ret = 0;
>> -	FILE *fp;
>> +	char controlgroup[1024], schema[1024], reason[128];
>> +	int resource_id, fp, ret = 0;
>
>fp -> fd to follow the usual convention.

Okay, I'll change it

>>  
>> -	fp = fopen(controlgroup, "w");
>> +	fp = open(controlgroup, O_WRONLY);
>>  	if (!fp) {
>>  		sprintf(reason, "Failed to open control group");
>>  		ret = -1;
>>  
>>  		goto out;
>>  	}
>> -
>> -	if (fprintf(fp, "%s\n", schema) < 0) {
>> -		sprintf(reason, "Failed to write schemata in control group");
>> -		fclose(fp);
>> +	if (write(fp, schema, strlen(schema)) < 0) {
>
>snprintf() returns you the length, just store the return value and there's 
>no need to calculate it here.

Thanks for the suggestion, tested it and it works fine, I'll add it in
the next version.

>> +		snprintf(reason, sizeof(reason),
>> +			 "write() failed : %s", strerror(errno));
>> +		close(fp);
>>  		ret = -1;
>>  
>>  		goto out;
>>  	}
>> -	fclose(fp);
>> +	close(fp);
>> +	schema[strcspn(schema, "\n")] = 0;
>
>Here too you can use the known length/index instead of strcspr().

Same as above

-- 
Kind regards
Maciej Wieczór-Retman