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

From: Lorenzo Stoakes <lstoakes@gmail.com>
To: Markus Elfring <Markus.Elfring@web.de>
Cc: kernel-janitors@vger.kernel.org, linux-kselftest@vger.kernel.org,
	cgroups@vger.kernel.org, linux-mm@kvack.org,
	Jay Kamat <jgkamat@fb.com>, Johannes Weiner <hannes@cmpxchg.org>,
	Michal Hocko <mhocko@kernel.org>,
	Muchun Song <muchun.song@linux.dev>,
	Roman Gushchin <roman.gushchin@linux.dev>,
	Shakeel Butt <shakeelb@google.com>, Shuah Khan <shuah@kernel.org>,
	Tejun Heo <tj@kernel.org>, Zefan Li <lizefan.x@bytedance.com>,
	cocci@inria.fr, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] selftests: cgroup: Fix exception handling in test_memcg_oom_group_score_events()
Date: Sat, 25 Mar 2023 19:24:08 +0000	[thread overview]
Message-ID: <fffcd98a-bb73-41cd-8545-0f2c55dd38f9@lucifer.local> (raw)
In-Reply-To: <c383bdca-6f0d-4a75-e788-e1920faa0a62@web.de>

On Sat, Mar 25, 2023 at 07:30:21PM +0100, Markus Elfring wrote:
> Date: Sat, 25 Mar 2023 19:11:13 +0100
>
> 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. Something like 'avoid unnecessary null
check/cg_destroy() invocation' would be far clearer.

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

This feels superfluious.

> 2. Use an additional label.

This also feels superfluious.

>
> 3. Delete a questionable check.

This seems superfluois and frankly, rude. It's not questionable, it's
readable, you should try to avoid language like 'questionable' when the
purpose of the check is obvious.

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

Fixes what in the what now? This is not a bug fix, it's a 'questionable'
refactoring.

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  tools/testing/selftests/cgroup/test_memcontrol.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
> index f4f7c0aef702..afcd1752413e 100644
> --- a/tools/testing/selftests/cgroup/test_memcontrol.c
> +++ 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;
> --
> 2.40.0
>
>

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 (!)

Not all pedantic Coccinelle results are actionable. Remember that it's
humans who are reading the code.

Your email client/scripting is still somehow broken, I couldn't get b4 to
pull it correctly and it seems to have a duplicate message ID. You really
need to take a look at that (git send-email should do this fine for
example).

Please try to filter the output of Coccinelle and instead of spamming
thousands of pointless patches that add no value, try to choose those that
do add value.

My advice overall would be to just stop.