Re: [PATCH V3 10/15] selftests/resctrl: Make benchmark parameter passing robust

[Reinette Chatre <reinette.chatre@intel.com>]

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