[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <104e159a-80fb-4ad5-ae9e-3e5f549a3535@intel.com>
Date: Fri, 18 Oct 2024 10:34:25 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC: <fenghua.yu@...el.com>, <shuah@...nel.org>, <tony.luck@...el.com>,
<peternewman@...gle.com>, <babu.moger@....com>,
Maciej Wieczór-Retman <maciej.wieczor-retman@...el.com>,
<linux-kselftest@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V3 10/15] selftests/resctrl: Make benchmark parameter
passing robust
Hi Ilpo,
On 10/18/24 2:03 AM, Ilpo Järvinen wrote:
> On Thu, 17 Oct 2024, Reinette Chatre wrote:
>> +/*
>> + * Allocate and initialize a struct fill_buf_param with user provided
>> + * (via "-b fill_buf <fill_buf parameters>") parameters.
>> + *
>> + * Use defaults (that may not be appropriate for all tests) for any
>> + * fill_buf parameters omitted by the user.
>> + *
>> + * Historically it may have been possible for user space to provide
>> + * additional parameters, "operation" ("read" vs "write") in
>> + * benchmark_cmd[3] and "once" (run "once" or until terminated) in
>> + * benchmark_cmd[4]. Changing these parameters have never been
>> + * supported with the default of "read" operation and running until
>> + * terminated built into the tests. Any unsupported values for
>> + * (original) "fill_buf" parameters are treated as failure.
>> + *
>> + * Return: On failure, forcibly exits the test on any parsing failure,
>> + * returns NULL if no parsing needed (user did not actually provide
>> + * "-b fill_buf").
>> + * On success, returns pointer to newly allocated and fully
>> + * initialized struct fill_buf_param that caller must free.
>> + */
>> +static struct fill_buf_param *alloc_fill_buf_param(struct user_params *uparams)
>> +{
>> + struct fill_buf_param *fill_param = NULL;
>> + char *endptr = NULL;
>> +
>> + if (!uparams->benchmark_cmd[0] || strcmp(uparams->benchmark_cmd[0], "fill_buf"))
>> + return NULL;
>> +
>> + fill_param = malloc(sizeof(*fill_param));
>> + if (!fill_param)
>> + ksft_exit_skip("Unable to allocate memory for fill_buf parameters.\n");
>> +
>> + if (uparams->benchmark_cmd[1]) {
>> + errno = 0;
>> + fill_param->buf_size = strtoul(uparams->benchmark_cmd[1], &endptr, 10);
>> + if (errno || *endptr != '\0') {
>
> Same here as with the patch 2 (and also in the checks below), both empty
> string and extra character checks are necessary.
Thank you very much. Addressed with fixup below:
@@ -181,7 +181,7 @@ static struct fill_buf_param *alloc_fill_buf_param(struct user_params *uparams)
if (!fill_param)
ksft_exit_skip("Unable to allocate memory for fill_buf parameters.\n");
- if (uparams->benchmark_cmd[1]) {
+ if (uparams->benchmark_cmd[1] && *uparams->benchmark_cmd[1] != '\0') {
errno = 0;
fill_param->buf_size = strtoul(uparams->benchmark_cmd[1], &endptr, 10);
if (errno || *endptr != '\0') {
@@ -192,7 +192,7 @@ static struct fill_buf_param *alloc_fill_buf_param(struct user_params *uparams)
fill_param->buf_size = MINIMUM_SPAN;
}
- if (uparams->benchmark_cmd[2]) {
+ if (uparams->benchmark_cmd[2] && *uparams->benchmark_cmd[2] != '\0') {
errno = 0;
fill_param->memflush = strtol(uparams->benchmark_cmd[2], &endptr, 10) != 0;
if (errno || *endptr != '\0') {
@@ -203,14 +203,14 @@ static struct fill_buf_param *alloc_fill_buf_param(struct user_params *uparams)
fill_param->memflush = true;
}
- if (uparams->benchmark_cmd[3]) {
+ if (uparams->benchmark_cmd[3] && *uparams->benchmark_cmd[3] != '\0') {
if (strcmp(uparams->benchmark_cmd[3], "0")) {
free(fill_param);
ksft_exit_skip("Only read operations supported.\n");
}
}
- if (uparams->benchmark_cmd[4]) {
+ if (uparams->benchmark_cmd[4] && *uparams->benchmark_cmd[4] != '\0') {
if (strcmp(uparams->benchmark_cmd[4], "false")) {
free(fill_param);
ksft_exit_skip("fill_buf is required to run until termination.\n");
Reinette
Powered by blists - more mailing lists