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

Hello!

On 2023-09-29 at 10:04:21 -0700, Reinette Chatre wrote:

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 sai.praneeth.prakhya@intel.com,
• Fenghua Yu fenghua.yu@intel.com

*/ +#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 */

Okay, I'll change it to the more typical error checking then. I suppose it's not possible to write zero characters to "schema" so my " < 1" won't be useful.

...

• 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

Thanks for pointing it out, I'll fix it for the next version.