Re: [PATCH v3 5/5] selftests/resctrl: Remove duplicate codes that clear each test result file

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

Hi Shaopeng,

On 11/8/2022 12:32 AM, Shaopeng Tan (Fujitsu) wrote:
> Hi Shuah and Reinette,
> 
>> On 11/1/2022 2:43 AM, Shaopeng Tan wrote:
>>> Before exiting each test function(run_cmt/cat/mbm/mba_test()),
>>> test results("ok","not ok") are printed by ksft_test_result() and then
>>> temporary result files are cleaned by function
>>> cmt/cat/mbm/mba_test_cleanup().
>>> However, before running ksft_test_result(), function
>>> cmt/cat/mbm/mba_test_cleanup() has been run in each test function as
>>> follows:
>>>    cmt_resctrl_val()
>>>    cat_perf_miss_val()
>>>    mba_schemata_change()
>>>    mbm_bw_change()
>>>
>>> Remove duplicate codes that clear each test result file.
>>
>> This isn't making much sense to me. Please include test report before and after
>> this change in the change log.
> 
> With or without this patch, there is no effect on the result message.
> These functions were executed twice, in brief, it runs as follows:
>  - cmt/cat/mbm/mba_test_cleanup()
>  - ksft_test_result()
>  - cmt/cat/mbm/mba_test_cleanup()
> So, I deleted once. 
> 
>> From what I can tell this still seem to suffer from the problem where the test
>> files may not be cleaned. With the removal of mbm_test_cleanup() the cleanup
>> is now expected to be done in mbm_bw_change().
>>
>> Note that:
>>
>> mbm_bw_change()
>> {
>> 	...
>>
>> 	ret = resctrl_val(benchmark_cmd, &param);
>> 	if (ret)
>> 		return ret;
>>
>> 	/* Test results stored in file */
>>
>> 	ret = check_results(span);
>> 	if (ret)
>> 		return ret; <== Return without cleaning test result file
>>
>> 	mbm_test_cleanup(); <== Test result file cleaned only when test
>> passed.
>>
>> 	return 0;
>> }
> 
> I intend to avoid this problem through the following codes.
> 
> mbm_bw_change()
> {
>         ret = resctrl_val(benchmark_cmd, &param);
>         if (ret)
> -               return ret;
> +               goto out;
> 
>         ret = check_results(span);
>         if (ret)
> -               return ret;
> +               goto out;
> 
> +out:
>         mbm_test_cleanup();
> 
> -       return 0;
> +       return ret;
> }
> 

Yes, even though file removal may now encounter ENOENT this
does seem the most robust route and the possible error is ok
since mbm_test_cleanup() does not check the return code.
Could you please replicate this pattern to the other functions
(mba_schemata_change() and cmt_resctrl_val()) also?

Reinette