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
