Re: [PATCH V3 02/15] selftests/resctrl: Print accurate buffer size as part of MBM results

[Reinette Chatre]

Hi Ilpo,

On 10/18/24 1:46 AM, Ilpo Järvinen wrote:
> On Thu, 17 Oct 2024, Reinette Chatre wrote:

>> @@ -138,15 +139,26 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
>>  		.setup		= mbm_setup,
>>  		.measure	= mbm_measure,
>>  	};
>> +	char *endptr = NULL;
>> +	size_t span = 0;
>>  	int ret;
>>  
>>  	remove(RESULT_FILE_NAME);
>>  
>> +	if (uparams->benchmark_cmd[0] && strcmp(uparams->benchmark_cmd[0], "fill_buf") == 0) {
>> +		if (uparams->benchmark_cmd[1]) {
>> +			errno = 0;
>> +			span = strtoul(uparams->benchmark_cmd[1], &endptr, 10);
>> +			if (errno || *endptr != '\0')
> 
> This no longer catches "" string as error. I tested strtoul() with an 
> empty string and errno remains at 0.
> 
>> +				return -errno;
> 
> Another issue is that in cases where errno=0 (both *endptr != '\0' and 
> endptr == uparams->benchmark_cmd[1]), this function doesn't return 
> a proper error code but -0.
> 

Thank you for catching this. I addressed it with the fixup below:

@@ -146,11 +146,11 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
 	remove(RESULT_FILE_NAME);
 
 	if (uparams->benchmark_cmd[0] && strcmp(uparams->benchmark_cmd[0], "fill_buf") == 0) {
-		if (uparams->benchmark_cmd[1]) {
+		if (uparams->benchmark_cmd[1] && *uparams->benchmark_cmd[1] != '\0') {
 			errno = 0;
 			span = strtoul(uparams->benchmark_cmd[1], &endptr, 10);
 			if (errno || *endptr != '\0')
-				return -errno;
+				return -EINVAL;
 		}
 	}

The above does not address all that can go wrong when user, for example, runs:
		resctrl_tests -t mbm fill_buf ""
The above snippet will ignore the invalid value and focus on the issue addressed in
this patch, which is to not print DEFAULT_SPAN when that is not the size used.

At this point of the series run_benchmark() still uses strtoul() to parse the
empty string. This is fixed in the later patch "selftests/resctrl: Make benchmark
parameter passing robust" that will also be fixed to handle the empty string.

Reinette