lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <fffcd98a-bb73-41cd-8545-0f2c55dd38f9@lucifer.local>
Date:   Sat, 25 Mar 2023 19:24:08 +0000
From:   Lorenzo Stoakes <lstoakes@...il.com>
To:     Markus Elfring <Markus.Elfring@....de>
Cc:     kernel-janitors@...r.kernel.org, linux-kselftest@...r.kernel.org,
        cgroups@...r.kernel.org, linux-mm@...ck.org,
        Jay Kamat <jgkamat@...com>,
        Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...nel.org>,
        Muchun Song <muchun.song@...ux.dev>,
        Roman Gushchin <roman.gushchin@...ux.dev>,
        Shakeel Butt <shakeelb@...gle.com>,
        Shuah Khan <shuah@...nel.org>, Tejun Heo <tj@...nel.org>,
        Zefan Li <lizefan.x@...edance.com>, cocci@...ia.fr,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] selftests: cgroup: Fix exception handling in
 test_memcg_oom_group_score_events()

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@...rs.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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ