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.
- Thus return directly after a call of the function “cg_name” failed.
This feels superfluious.
- Use an additional label.
This also feels superfluious.
- 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.