Re: [cocci] [PATCH] selftests: cgroup: Fix exception handling in test_memcg_oom_group_score_events()

[Markus Elfring <Markus.Elfring@web.de>]

>> The label “cleanup” was used to jump to another pointer check despite of
>> the detail in the implementation of the function
>> “test_memcg_oom_group_score_events” that it was determined already
>> that a corresponding variable contained a null pointer.
>
> This is poorly writte and confusing.

A special source code combination was detected.


Reconsidering repeated pointer checks with SmPL
https://lore.kernel.org/cocci/f9303bdc-b1a7-be5e-56c6-dfa8232b8b55@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00017.html



> Something like 'avoid unnecessary null check/cg_destroy() invocation'
> would be far clearer.

I (or another interested contributor) can integrate such information into
a subsequent patch if the change acceptance will grow accordingly.


>> 1. Thus return directly after a call of the function “cg_name” failed.
>
> This feels superfluious.

Under which circumstances would you care more for the advice
“If there is no cleanup needed then just return directly.”
from the section “Centralized exiting of functions”?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.3-rc3#n532


>> 2. Use an additional label.
>
> This also feels superfluious.

The marking of special source code positions is probably required for
the presented design goal.


>> 3. Delete a questionable check.
>
> This seems superfluois and frankly, rude. It's not questionable,

Can you find the combination of code fragments like “if (!memcg)”
and “if (memcg)” suspicious?


> it's readable, you should try to avoid language like 'questionable' when the
> purpose of the check is obvious.

I tend to prefer an other known design variant at this place.


>> This issue was detected by using the Coccinelle software.
>>
>> Fixes: a987785dcd6c8ae2915460582aebd6481c81eb67 ("Add tests for memory.oom.group")
>
> Fixes what in the what now?

1. Check repetition (which can be undesirable)

2. Can a cg_destroy() call ever work as expected if a cg_create() call failed?


>> +++ b/tools/testing/selftests/cgroup/test_memcontrol.c
>> @@ -1242,12 +1242,11 @@ static int test_memcg_oom_group_score_events(const char *root)
>>  	int safe_pid;
>>
>>  	memcg = cg_name(root, "memcg_test_0");
>> -
>>  	if (!memcg)
>> -		goto cleanup;
>> +		return ret;
>>
>>  	if (cg_create(memcg))
>> -		goto cleanup;
>> +		goto free_cg;
>>
>>  	if (cg_write(memcg, "memory.max", "50M"))
>>  		goto cleanup;
>> @@ -1275,8 +1274,8 @@ static int test_memcg_oom_group_score_events(const char *root)
>>  	ret = KSFT_PASS;
>>
>>  cleanup:
>> -	if (memcg)
>> -		cg_destroy(memcg);
>> +	cg_destroy(memcg);
>> +free_cg:
>>  	free(memcg);
>>
>>  	return ret;
>> --
…
> I dislike this patch, it adds complexity for no discernible purpose and
> actively makes the code _less_ readable and in a self-test of all places (!)

It seems that there is a conflict to resolve also according to another
information source.
https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources#MEM12C.Considerusingagotochainwhenleavingafunctiononerrorwhenusingandreleasingresources-CompliantSolution%28POSIX,GotoChain%29


Regards,
Markus