Hi Maciej, On 9/29/2023 1:21 AM, Maciej Wieczor-Retman wrote: ...
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c > b/tools/testing/selftests/resctrl/resctrlfs.c > index 3a8111362d26..342a3dbcdbb6 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], schema[1024], reason[128]; > + int resource_id, fd, schema_len = 0, 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 < 1) { I find that this complicates the code since this is not the typical snprintf() error checking (0 is a valid snprintf() return). I think it will make the code easier to understand if it sticks to snprintf() error checking and initialize schema_len to match. What I mean with this is something like this: int schema_len = -1; if (...) schema_len = snprintf(...); if (schema_len < 0 || schema_len >= sizeof(schema)) /* error handling */ > + 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", Also please note that _if_ there is an early exit then uninitialized schema will be printed. Maybe this also needs a schema[1024] = {} ? Reinette
