Hi Maciej,
On 9/22/2023 1:10 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>
> ---
> 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 | 30 ++++++++++++---------
> 1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index 3a8111362d26..edc8fc6e44b0 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], schema[1024], reason[128];
> + int resource_id, fd, schema_len = -1, ret = 0;
I am trying to understand the schema_len initialization. Could
you please elaborate why you chose -1? I'm a bit concerned with
the robustness here with it being used as an unsigned integer
in write() and also the negative array index later.
>
> if (strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) &&
> strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) &&
> @@ -520,27 +520,31 @@ 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);
>
> - fp = fopen(controlgroup, "w");
> - if (!fp) {
> - sprintf(reason, "Failed to open control group");
> + fd = open(controlgroup, O_WRONLY);
> + if (!fd) {
Be careful ... the error checking appropriate to the original
pointer needs a double check with this new usage.
According to "man 2 open" - open() returns -1 on error so I expect
that this should rather be:
if (fd < 0) {
or
if (fd == -1) {
The rest looks good to me.
Reinette
