Re: [PATCH V2 09/13] selftests/resctrl: Make benchmark parameter passing robust

[Reinette Chatre]

Hi Ilpo,

On 10/4/24 7:05 AM, Ilpo Järvinen wrote:
> On Thu, 12 Sep 2024, Reinette Chatre wrote:

>>  tools/testing/selftests/resctrl/cmt_test.c    |  32 ++----
>>  tools/testing/selftests/resctrl/mba_test.c    |  13 ++-
>>  tools/testing/selftests/resctrl/mbm_test.c    |  22 ++--
>>  tools/testing/selftests/resctrl/resctrl.h     |  54 ++++++---
>>  .../testing/selftests/resctrl/resctrl_tests.c | 104 +++++++++++++-----
>>  tools/testing/selftests/resctrl/resctrl_val.c |  43 ++++----
>>  6 files changed, 176 insertions(+), 92 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
>> index 0c045080d808..a7effe76b419 100644
>> --- a/tools/testing/selftests/resctrl/cmt_test.c
>> +++ b/tools/testing/selftests/resctrl/cmt_test.c
>> @@ -116,15 +116,13 @@ static void cmt_test_cleanup(void)
>>  
>>  static int cmt_run_test(const struct resctrl_test *test, const struct user_params *uparams)
>>  {
>> -	const char * const *cmd = uparams->benchmark_cmd;
>> -	const char *new_cmd[BENCHMARK_ARGS];
>> +	struct fill_buf_param fill_buf = {0};
> 
> Empty initializer is enough to have the fields initialized to zero.

Sure.

...

>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
>> index 82801245e4c1..98462752cb46 100644
>> --- a/tools/testing/selftests/resctrl/resctrl.h
>> +++ b/tools/testing/selftests/resctrl/resctrl.h
>> @@ -43,16 +43,35 @@
>>  
>>  #define DEFAULT_SPAN		(250 * MB)
>>  
>> +/*
>> + * fill_buf_param:	"fill_buf" benchmark parameters
>> + * @buf_size:		Size (in bytes) of buffer used in benchmark.
>> + *			"fill_buf" allocates and initializes buffer of
>> + *			@buf_size. User can change value via command line.
>> + * @memflush:		If 0 then the buffer will not be flushed after
>> + *			allocation and initialization, otherwise the
>> + *			buffer will be flushed. User can change value via
>> + *			command line.
>> + */
>> +struct fill_buf_param {
>> +	size_t		buf_size;
>> +	int		memflush;
> 
> bool?
> 
> Or is there a plan to use other values than 0 and 1?

Sure. There is no need to keep the original user provided values. I am not aware of
any other planned values. 

>> +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 || uparams->benchmark_cmd[1] == endptr) {
> 
> Should this also check that there is no extra garbage?

Good point. Instead of adding another check I plan to replace existing endptr check
with a different test that ensures that "*endptr == '\0'".


>>  static void run_benchmark(int signum, siginfo_t *info, void *ucontext)
>>  {
>> -	char **benchmark_cmd;
>> -	int ret, memflush;
>> -	size_t span;
>> +	struct benchmark_info *benchmark_info;
>> +	const struct user_params *uparams;
>> +	struct resctrl_val_param *param;
>>  	FILE *fp;
>> +	int ret;
>>  
>> -	benchmark_cmd = info->si_ptr;
>> +	benchmark_info = info->si_ptr;
> 
> I'd just assign this directly while defining the local variable.
> 

Sure.

Thank you.

Reinette