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