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 <maciej.wieczor-retman@xxxxxxxxx>
>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
>> ---
>> 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 <sai.praneeth.prakhya@xxxxxxxxx>,
>> * Fenghua Yu <fenghua.yu@xxxxxxxxx>
>> */
>> +#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 <reinette.chatre@xxxxxxxxx>
>
>Reinette
Thanks, I'll make the correction and resend.
--
Kind regards
Maciej Wieczór-Retman
