From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8F002C6FD1C for ; Sun, 26 Mar 2023 08:16:12 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D9FD76B0072; Sun, 26 Mar 2023 04:16:11 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D4F6E6B0074; Sun, 26 Mar 2023 04:16:11 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BCB476B0075; Sun, 26 Mar 2023 04:16:11 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id A6B396B0072 for ; Sun, 26 Mar 2023 04:16:11 -0400 (EDT) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 663E6403C4 for ; Sun, 26 Mar 2023 08:16:11 +0000 (UTC) X-FDA: 80610341742.09.FA60EB4 Received: from mout.web.de (mout.web.de [212.227.17.11]) by imf20.hostedemail.com (Postfix) with ESMTP id 2B8E21C0008 for ; Sun, 26 Mar 2023 08:16:08 +0000 (UTC) Authentication-Results: imf20.hostedemail.com; dkim=pass header.d=web.de header.s=s29768273 header.b=i60JcdfE; spf=pass (imf20.hostedemail.com: domain of Markus.Elfring@web.de designates 212.227.17.11 as permitted sender) smtp.mailfrom=Markus.Elfring@web.de; dmarc=pass (policy=none) header.from=web.de ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1679818569; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=0hOXMUsTjQwfIGkRK6IQA5bCvVxoMO8D/eIb/X7Jn6o=; b=uaDeJNnmCHnDZyq1f75cs4LiYAKMYQOoZxhhW7NuFOg1KAHGTCD+qPvMUZc09DaQltFPlr qK/OlxevVUDfcjSLer45bV0t/4FAK88Pl9eAXoLSBHxZEplmDl+c9kLlX5wNts9qIPmXhv H2eg5nru/jywS0CgTJieiW3E8npJ+cg= ARC-Authentication-Results: i=1; imf20.hostedemail.com; dkim=pass header.d=web.de header.s=s29768273 header.b=i60JcdfE; spf=pass (imf20.hostedemail.com: domain of Markus.Elfring@web.de designates 212.227.17.11 as permitted sender) smtp.mailfrom=Markus.Elfring@web.de; dmarc=pass (policy=none) header.from=web.de ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1679818569; a=rsa-sha256; cv=none; b=0JcYXx5bJ/2mfwyDbVW85mLzhtwOrQEnv4vsQDi+zwyf6xgJ3xurBGV7H3x2oXhj6qZqqe Yjku8GK9tsikhX67cOlVi+WVjLvsrhcvLXref5uLdybOtE3mx2rerUCs3tAIrh0D0LVoqE LntCQgtmsuig/sA9aUzvl1APZ11IniI= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=web.de; s=s29768273; t=1679818543; i=markus.elfring@web.de; bh=rBCEKqUiIg95hHqIQA+rJDnVzCS8UUItPB3j6Z3IkBs=; h=X-UI-Sender-Class:Date:Subject:To:Cc:References:From:In-Reply-To; b=i60JcdfEiXZjhXmlN0wT0hEYONtQ+sM4g9Oc04ByPbsuIwSNCVKKVvSZUmE7VPD/t ypNOoxI2sA+d/y4T+WeAuuicPGKuEfgiK1h1Lm02Ofc0bNBgZvnwzf4xGkwhzR0CO5 D2OlYCmR/A0O9pRPh38FVNdjhw6uss8m39rqE+KhIofExgH0pTW3R2IqaNMmjPct8F QBXvq3FK8NKjGuR9tQemjYVP/aAqzLElpD00tI/LGUGhI/bKOkd1cQrQVf5kjZ5Lp6 NtcFb2R0zDaob9RnSoJMcNxDqTCqx2GcxnlA+PA9DFq7/zsHxz7Zz5P2gFKshenPnW yiLxGI2/7HzUg== X-UI-Sender-Class: 814a7b36-bfc1-4dae-8640-3722d8ec6cd6 Received: from [192.168.178.21] ([94.31.81.83]) by smtp.web.de (mrweb106 [213.165.67.124]) with ESMTPSA (Nemesis) id 1MCGWe-1poYjU3ycM-009t05; Sun, 26 Mar 2023 10:15:43 +0200 Message-ID: <5b7921c9-ee5d-c372-b19b-2701bcf33148@web.de> Date: Sun, 26 Mar 2023 10:15:31 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Subject: Re: [PATCH] selftests: cgroup: Fix exception handling in test_memcg_oom_group_score_events() To: Lorenzo Stoakes , kernel-janitors@vger.kernel.org, linux-kselftest@vger.kernel.org, cgroups@vger.kernel.org, linux-mm@kvack.org, Jay Kamat , Johannes Weiner , Michal Hocko , Muchun Song , Roman Gushchin , Shakeel Butt , Shuah Khan , Tejun Heo , Zefan Li Cc: cocci@inria.fr, LKML References: Content-Language: en-GB From: Markus Elfring In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Provags-ID: V03:K1:CBS7TQ0coEyfU1zDX6UzZ2c/DSGRYUqAGUO/MwxFDUlXHAIwJTZ YGkX2BVXs1uGwMHGfSVafOqgELFusnWi4KQLjfJKPCBKxll66XQjb3G5CTAhoDplh5x8hvW /3yb/IyQorfWfb9jiRjhnFNzR5LJPsnt0QhZ48n3DCdP+WsOKGhuvrbq6EX0Q/R4knM3Wdh JrD8qnTmgTo/JpMJLF2nw== UI-OutboundReport: notjunk:1;M01:P0:+RjmvXaqkvE=;SYhxCrqIkuIrPPvQxrSyYTbv/wH Ez4hZm2uNpSph3k+J0AxcSNpAeqPnccThwQLiIy1zyHKTwLB9i3E4LHC1cLYLdvPa/3zkxp4b 8wEoFWDIAYDsHeBZSPbzjR33nS2IXV+kvz4F8ORPOWjAKRIVunwekmnjMHq9sLHzIV/GqZYzJ uCKcn6a9z+8r6wOAV8h+kLowO2OVGb1R1Xezbo+FjZyY2i5QgCVWZdi6My0JSszoRjnPx9ORG L2tZT4Yed293arWn4nsUoaPLd+WStPxH5gyp5Fn+5QPo+fQkRsuPHSvwW4wyRju46ezN3FNUM yXdRuj/nLN42M0f84hkU5uKd5iD8amHoysmAuk1eRBhR1Q6foGYdCPRPHLMpE9BCWeWlksDKb LlY5Vaasp6KjEp4xNwtQCgOTIMgIgfCweFupddWR0JR4ARBOp/E/wvcO1YQ6h+sUohEPj+WzS SmGwdzlBnCHcufxPCFde3EOADnNSLg1X+4jtv3iE8AumpCNW4j8+pxHNsUbc1kbIg+kJIgP8k K/B9kuvRSYbKOyK5wMJ7qfluogrN81niDtrry6+Rin95pxs6BTjEll6tl1iCHx/FYjWyvqlY5 DUFQcyaY01ZpmxJcCczIp1ozP0utZKtYfuzfwEjWo2RoKcaZ3hZHw00plXJcPT8/RZMTSh5qx NxjDDfVxn/LSbe5gINEcQPM6LbW92qjF03N5Ej5iXjAqZ8/VuCwQ6h2vzntwONISHBQUzMqJZ u0hC6A9HHR/ppFlnMHmLmclxCaKVystMU0gnwJk7t0HZywdN+O1c6435hdNkfaiPjRUeRWm30 eHt/9jDH4+hru0yr5Dx0W8t1JqJ4gg5eRSLowvG9OnLn+vCGWc+RjEk65pbV4JqH6IFV2G1Tm wry33gAq788wgld7i8e0/EWhSkFJRv2CgdSysM17hMbwSN2HZ8ZiimXC1 X-Stat-Signature: 59qr87gwr4gqawrd375j87x798poewoo X-Rspam-User: X-Rspamd-Queue-Id: 2B8E21C0008 X-Rspamd-Server: rspam06 X-HE-Tag: 1679818568-417785 X-HE-Meta: U2FsdGVkX19ym5Q6BlYO746VPNbB+pJNFV6nF2FP/6M6fTwm4K0A3yzhAcIHDkx5HEG4wdQsUn3QejCB5/j1hwShkkca5mgk3AYqo4u3JP6l3grWtQDLINBipOLmz1AY/ZuiUE9W4VXv7YvLtTFx8OMXBhra5tq2uRVS/CB6feemFlzMoSJaOpkL1PStt1o1G4ZiLfWWiD/turFaTB4iYXeRUdbCVxxJE001pkslLTd4xXvnRU73fbBei0OAp5Mb32Tybk979mZxn33CwH7fk0LFH3etJ1aOORsM9HZaZp7CTc/KAzRbGDzbcMT/vKGdPCBd675c3UiTvs4pMee6I2f74WaVmaAR0phpH9+HAbSluopmFmIgRKaGEORMT17d2ZgFpgnBiNDecV7huflZlXDMC1BFtMaJPlrPlo3eIGbEvMAx8uwWAmr9Ana0nbPcohFAHEe2ox4BTt3GSj14F1H+4oXP5mfidTnxfM4E7gCzzUddx3KrjpB8cYvA43fCI0piBV8XnFSPxrk00sVXJyAHgS+PmoUTpGhWM7FyYHYFzQDO4ez4VhzXNPESrtJS7uVT8gURJ+7dkSDmvUKk9nnTxj/qxw93zXq0U207rtR464TO5ejtUttWVjfv2DufJtfdSLKEjWh0TA0iQ2kf3xabBx9t5elGbaKCMjStZIbbC6bzhxbOABc46JVph4RkHIFv4oHpR6GOArGLPQgIhacW1LyVzbJmYhpilSDEqu+E8gjwrUxaZbklPT4yrLmswYwT5JOAqUUiiehnBlMREzM/7/irIlpcFS1ZXozSdIXtWnpEsTJ9agSt1T47BIggfINOgelf4DU+aunPqHac+6xDAGIFJKiP28EP5al0vi5FAq1rSKQV7jLuurI6rvM9iixKBIWfvfm5d3T1NU7nZ3N6b4OjrZ9AbNk9YfCYMHzhU8C2OP6/JCI8b/YvMTkj3YHsuEpVBzAlAYRCEh/ I0CLB3s1 oPZy56dO0+WyyPxWB0xYemOXiIau8cGqtWj24jZbeGUyt96kHUGdKdFwu0mDykxCJN//nVNBX+nO7xa3Wd82WPccnXAKx1Uv0M+0ER6ZK309RMjtkD11ViLcs1m+HDYFEy7rYfERxgLnuL1plYsf2LU8KdJV7sD1k43JvOq82QXxy8JpWhBfKOmU+Q3XYjtIuO8MtrHSnpAWDhbxFtz1NyMFEcYkTc3ryibv2pXQcJvX8pCQHlSxJK7QpJo74kKG5NdqV4tR12hKhYVh1S1pHDd2YQLzEq2Wb+50IZJINj9DnupkCbctFKcQQU19SemHQTLD0atnUYrD76iogKGkJhmO8RAtXux/HdyaSb7dx6qJsV8gnvJl61vgN8eKmjcPMK8LlVM+W/RJrL3hNxA9ya4nrjA== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: >> The label =E2=80=9Ccleanup=E2=80=9D was used to jump to another pointer= check despite of >> the detail in the implementation of the function >> =E2=80=9Ctest_memcg_oom_group_score_events=E2=80=9D that it was determi= ned 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 =E2=80=9Ccg_name= =E2=80=9D failed. > > This feels superfluious. Under which circumstances would you care more for the advice =E2=80=9CIf there is no cleanup needed then just return directly.=E2=80=9D from the section =E2=80=9CCentralized exiting of functions=E2=80=9D? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Do= cumentation/process/coding-style.rst?h=3Dv6.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 =E2=80=9Cif (!memcg)= =E2=80=9D and =E2=80=9Cif (memcg)=E2=80=9D 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 fai= led? >> +++ b/tools/testing/selftests/cgroup/test_memcontrol.c >> @@ -1242,12 +1242,11 @@ static int test_memcg_oom_group_score_events(co= nst char *root) >> int safe_pid; >> >> memcg =3D 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(cons= t char *root) >> ret =3D KSFT_PASS; >> >> cleanup: >> - if (memcg) >> - cg_destroy(memcg); >> + cg_destroy(memcg); >> +free_cg: >> free(memcg); >> >> return ret; >> -- =E2=80=A6 > 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+go= to+chain+when+leaving+a+function+on+error+when+using+and+releasing+resourc= es#MEM12C.Considerusingagotochainwhenleavingafunctiononerrorwhenusingandre= leasingresources-CompliantSolution%28POSIX,GotoChain%29 Regards, Markus