On 2023-10-09 at 11:55:16 -0700, Reinette Chatre wrote: >Hi Maciej, > >On 10/9/2023 3:57 AM, Maciej Wieczor-Retman wrote: >> Writing bitmasks to the schemata can fail when the bitmask doesn't >> adhere to 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(). >> >> Remove newline character from the schema 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. >> >> Signed-off-by: Maciej Wieczor-Retman <[email protected]> >> Reviewed-by: Ilpo Järvinen <[email protected]> >> --- >> Changelog v6: >> - Align schema_len error checking with typical snprintf format. >> (Reinette) >> - Initialize schema string for early return eventuality. (Reinette) >> >> Changelog v5: >> - Add Ilpo's reviewed-by tag. >> - Fix wrong open() error checking. (Reinette) >> - Add error checking to schema_len variable. >> >> Changelog v4: >> - Unify error checking between open() and write(). (Reinette) >> - Add fcntl.h for glibc backward compatiblitiy. (Reinette) >> >> Changelog v3: >> - Rename fp to fd. (Ilpo) >> - Remove strlen, strcspn and just use the snprintf value instead. (Ilpo) >> >> Changelog v2: >> - Rewrite patch message. >> - Double "reason" buffer size to fit longer error explanation. >> - Redo file interactions with syscalls instead of stdio functions. >> >> tools/testing/selftests/resctrl/resctrlfs.c | 36 +++++++++++++-------- >> 1 file changed, 23 insertions(+), 13 deletions(-) >> >> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c >> b/tools/testing/selftests/resctrl/resctrlfs.c >> index 3a8111362d26..648f9ec8b355 100644 >> --- a/tools/testing/selftests/resctrl/resctrlfs.c >> +++ b/tools/testing/selftests/resctrl/resctrlfs.c >> @@ -8,6 +8,7 @@ >> * Sai Praneeth Prakhya <[email protected]>, >> * Fenghua Yu <[email protected]> >> */ >> +#include <fcntl.h> >> #include <limits.h> >> >> #include "resctrl.h" >> @@ -490,9 +491,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], reason[128], schema[1024] = {}; >> + int resource_id, fd, schema_len = -1, ret = 0; >> >> if (strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) && >> strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) && >> @@ -520,27 +520,37 @@ 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); >> + schema_len = 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); >> + schema_len = snprintf(schema, sizeof(schema), "%s%d%c%s\n", >> + "MB:", resource_id, '=', schemata); >> + if (schema_len < 0 || schema_len >= sizeof(schema)) { >> + snprintf(reason, sizeof(reason), >> + "snprintf() failed with return value : %d", >> schema_len); >> + ret = -1; >> + goto out; >> + } >> >> - fp = fopen(controlgroup, "w"); >> - if (!fp) { >> - sprintf(reason, "Failed to open control group"); >> + fd = open(controlgroup, O_WRONLY); >> + if (fd < 0) { >> + snprintf(reason, sizeof(reason), >> + "open() failed : %s", strerror(errno)); >> ret = -1; >> >> goto out; >> } >> - >> - if (fprintf(fp, "%s\n", schema) < 0) { >> - sprintf(reason, "Failed to write schemata in control group"); >> - fclose(fp); >> + if (write(fd, schema, schema_len) < 0) { >> + snprintf(reason, sizeof(reason), >> + "write() failed : %s", strerror(errno)); >> + close(fd); >> ret = -1; >> >> goto out; >> } >> - fclose(fp); >> + close(fd); >> + schema[schema_len - 1] = 0; >> >> out: >> ksft_print_msg("Write schema \"%s\" to resctrl FS%s%s\n", > > >As changelog states, the newline is removed from schema to >ensure it is printed correctly. Note that this is not done when an >error is encountered during open() or write() so when an error is >encountered in these places then the print does not look as intended. > >I think a new goto label inserted just before the newline removal >should be sufficient, with the open() and write() error paths jumping >to it. > >With that addressed: > >Reviewed-by: Reinette Chatre <[email protected]> > >Reinette
Thanks, I'll make the correction and resend. -- Kind regards Maciej Wieczór-Retman
