Re: [PATCH v5 4/5] selftests/resctrl: Cleanup properly when an error occurs in CAT test

[Reinette Chatre]

Hi Shaopeng,

On 1/23/2023 6:16 PM, Shaopeng Tan (Fujitsu) wrote:
>> On 1/22/2023 8:22 PM, Shaopeng Tan (Fujitsu) wrote:
>>>> On 1/10/2023 11:58 PM, Shaopeng Tan wrote:
>>>>> diff --git a/tools/testing/selftests/resctrl/cat_test.c
>>>>> b/tools/testing/selftests/resctrl/cat_test.c
>>>>> index 6a8306b0a109..87302b882929 100644
>>>>> --- a/tools/testing/selftests/resctrl/cat_test.c
>>>>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>>>>> @@ -103,7 +103,6 @@ int cat_perf_miss_val(int cpu_no, int n, char
>>>> *cache_type)
>>>>>  	unsigned long l_mask, l_mask_1;
>>>>>  	int ret, pipefd[2], sibling_cpu_no;
>>>>>  	char pipe_message;
>>>>> -	pid_t bm_pid;
>>>>>
>>>>>  	cache_size = 0;
>>>>>
>>>>> @@ -181,28 +180,29 @@ int cat_perf_miss_val(int cpu_no, int n, char
>>>> *cache_type)
>>>>>  		strcpy(param.filename, RESULT_FILE_NAME1);
>>>>>  		param.num_of_runs = 0;
>>>>>  		param.cpu_no = sibling_cpu_no;
>>>>> +	} else {
>>>>> +		ret = signal_handler_register();
>>>>> +		if (ret)
>>>>> +			goto out;
>>>>
>>>> The "goto" will unregister the signal handler. Is that necessary if
>>>> the registration failed?
>>>>
>>>> Also, if signal_handler_register() fails then the child will keep
>>>> running and run its test ... would child not then run forever?
>>>
>>> A signal handler is needed here, but it is rarely used.
>>> Also, the registration rarely fails.
>>> Therefore, if registration failed,
>>> just print a warning/info message as follow.
>>> how about this idea?
>>>
>>> ksft_print_msg("Failed to register signal handler, signals
>>> SIGINT/SIGTERM/SIGHUP will not be handled as expected");
>>>
>>
>> I do not think this is necessary considering that signal_handler_register()
>> already prints an error on failure.
>>
>> Adding an error message does not address the two issues I raised.
> 
> The previous idea was just to print a message instead of "goto".
> How about the idea to keep the parent&child process running forever even if the signal handler registration fails.
> 
> +	} else {
> +		signal_handler_register();
> +	}
> 
> I don't think the test need stop when the signal handler registration fails,
> since this signal handler is rarely used and registration of signal handlers rarely fails.

Please always handle errors properly. If the registration fails the test should be
stopped and proper cleanup should be done. 

Reinette