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 A7A48C433EF for ; Tue, 19 Jul 2022 17:28:15 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1F4636B0071; Tue, 19 Jul 2022 13:28:15 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 17E636B0073; Tue, 19 Jul 2022 13:28:15 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 01E666B0074; Tue, 19 Jul 2022 13:28:14 -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 DF9976B0071 for ; Tue, 19 Jul 2022 13:28:14 -0400 (EDT) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id B3398C0137 for ; Tue, 19 Jul 2022 17:28:14 +0000 (UTC) X-FDA: 79704532908.12.B85BBB9 Received: from mail-wr1-f41.google.com (mail-wr1-f41.google.com [209.85.221.41]) by imf07.hostedemail.com (Postfix) with ESMTP id 62D4D40059 for ; Tue, 19 Jul 2022 17:28:14 +0000 (UTC) Received: by mail-wr1-f41.google.com with SMTP id d16so22621785wrv.10 for ; Tue, 19 Jul 2022 10:28:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=JGWTrJ/A2ihWU/Ug7aQjRrDit++Zh+gpJdGLbqVf1ag=; b=aQcLJHLSmAz57vMuBvrLAMuiJ9nsj6bTyOPDKgofJNdmDLyZvDjGqvPFf8eyewoclH l1l5WETYZVlZ8uJmvS1H1ti0C5X06q29fxB0wCYYKPJxZfRH94Xb1Mn5EMR6/0o1UY8j 145zovNXLJSR4DnmMp+8wX78v/m524bvc3tJ+FzQf0nT7yT7yXfSyHDUUj3Hy0aaa4lA 2P6GzyXXUlsVQ6hDfqddxLzfkhz8yvCLArXMHBH3qtEjNqAPGY+Meno0NzZjndBqiLbZ KocO65DubKojur00kb9JuLMzyBK5C5SHKSqew69Pcn4b2KCT1iox6ILhl/V1vBrI7fDX yCBg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=JGWTrJ/A2ihWU/Ug7aQjRrDit++Zh+gpJdGLbqVf1ag=; b=cdMsiydRU60tMb8C/1jLNATz28VVW2eT2DamSNyVbOQf29jGwvtKVKtmdqquSPmlR0 wCG0YxNjK6pPSx4wqMPVxGJ4pEOjLzocv9shepO/S1CneKZAU4f2vv3qtspsN+5wkuBJ DcbbjLVWpLJkslByVPhNQvBc9m2YftBqncDZrH1OfgkOaS5mP0YtZnndnIWuKanecZpu Y3d/boVYnwJ0p6jvJxIY1CIKxWF5DigXWPW61U8F8i4UE5a6pRhGrNyMnNxchZovI6bs eWUolX4bhUEdsSr9gwDzmq0qs1ehYnSHI9+oixHEXDUteHezu0w92nlgXuBr0rZkAPlH Po+w== X-Gm-Message-State: AJIora878QpQatsxCJXQ5a+egJE968ulPI6QhKnRJKO+luHVXCNN3J8Z c+i7XqZWvEvo8gGjaBQR8Ta0PLR3wkTyV6qsi/IinQ== X-Google-Smtp-Source: AGRyM1tBJsBbqZfOaPheraq2qdKwFMuN2cYbU2/3gDb2ExcHHWknED0R67mCoawJmgennwfCC5j2IF55QsfrJAGvaF0= X-Received: by 2002:a5d:5a82:0:b0:21e:2899:60bd with SMTP id bp2-20020a5d5a82000000b0021e289960bdmr5795825wrb.80.1658251692855; Tue, 19 Jul 2022 10:28:12 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Yosry Ahmed Date: Tue, 19 Jul 2022 10:27:36 -0700 Message-ID: Subject: Re: [PATCH] selftests: memcg: uninitialized variable in test_memcg_reclaim() To: Dan Carpenter Cc: Johannes Weiner , Michal Hocko , Roman Gushchin , Shakeel Butt , Muchun Song , Tejun Heo , Zefan Li , Shuah Khan , David Rientjes , Andrew Morton , Cgroups , Linux-MM , linux-kselftest@vger.kernel.org, kernel-janitors@vger.kernel.org Content-Type: text/plain; charset="UTF-8" ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=aQcLJHLS; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf07.hostedemail.com: domain of yosryahmed@google.com designates 209.85.221.41 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1658251694; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=JGWTrJ/A2ihWU/Ug7aQjRrDit++Zh+gpJdGLbqVf1ag=; b=L6h4DMwYbi4EQEzlQQQ4JGGv5pMU9gOlpSmBvnbwudry8BfF+eWNAuDJj64Zcll95TjEpR ZELnLqNinWSQx/Az7qGdN+DoX/MrKjmItYt15Gs+eJCbWPKfBUedaRcHYTvToaE27PkH+R xmoIux8W+wqE9fw8tCAX2kOlM4BHZ4o= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1658251694; a=rsa-sha256; cv=none; b=hYlpthw+CkwcOqqAbxhr5aeZH8crbOkNpvw8i10opfHQtgQXcxK0GO6mJRMUMQVV2UyZDh Wwf3iwXXDn+DdP5cdaoEZ/BKYxFlJNPQwd0f3z+L0TIPNQu2bo1p8lWPyG7z4Q0ksolwHH ikPVueFMrOPvUKiDMrYIKtYjPagqrPQ= X-Rspamd-Queue-Id: 62D4D40059 Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=aQcLJHLS; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf07.hostedemail.com: domain of yosryahmed@google.com designates 209.85.221.41 as permitted sender) smtp.mailfrom=yosryahmed@google.com X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: h8a5nfomztth4pjebcymr8n6zdkmyt9f X-HE-Tag: 1658251694-937586 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: Hi Dan! On Tue, Jul 19, 2022 at 2:46 AM Dan Carpenter wrote: > > The "fd" is used on the clean up path without ever being initialized. > > Fixes: eae3cb2e87ff ("selftests: cgroup: add a selftest for memory.reclaim") Thanks for fixing this :) > Signed-off-by: Dan Carpenter > --- > I kind of went over kill on fixing this as if it were real code which > matters. :P > > .../selftests/cgroup/test_memcontrol.c | 23 +++++++++++-------- > 1 file changed, 13 insertions(+), 10 deletions(-) > > diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c > index 8833359556f3..08681699c2f9 100644 > --- a/tools/testing/selftests/cgroup/test_memcontrol.c > +++ b/tools/testing/selftests/cgroup/test_memcontrol.c > @@ -658,18 +658,18 @@ static int test_memcg_reclaim(const char *root) > > memcg = cg_name(root, "memcg_test"); > if (!memcg) > - goto cleanup; > + return KSFT_FAIL; Nit: Just goto free here as well, free ignores NULLs anyway. It's easier to have fewer return paths and more consistent with other tests. > > if (cg_create(memcg)) > - goto cleanup; > + goto free_memcg; > > current = cg_read_long(memcg, "memory.current"); > if (current != 0) > - goto cleanup; > + goto destroy_memcg; > > fd = get_temp_fd(); > if (fd < 0) > - goto cleanup; > + goto destroy_memcg; > > cg_run_nowait(memcg, alloc_pagecache_50M_noexit, (void *)(long)fd); > > @@ -697,7 +697,7 @@ static int test_memcg_reclaim(const char *root) > fprintf(stderr, > "failed to allocate %ld for memcg reclaim test\n", > expected_usage); > - goto cleanup; > + goto close; > } > } > > @@ -717,7 +717,7 @@ static int test_memcg_reclaim(const char *root) > * not reclaim the full amount. > */ > if (to_reclaim <= 0) > - goto cleanup; > + goto close; > > > snprintf(buf, sizeof(buf), "%ld", to_reclaim); > @@ -729,7 +729,7 @@ static int test_memcg_reclaim(const char *root) > */ > current = cg_read_long(memcg, "memory.current"); > if (!values_close(current, MB(30), 3) && current > MB(30)) > - goto cleanup; > + goto close; > break; > } > > @@ -738,14 +738,17 @@ static int test_memcg_reclaim(const char *root) > continue; > > /* We got an unexpected error or ran out of retries. */ > - goto cleanup; > + goto close; > } > > ret = KSFT_PASS; > -cleanup: > + > +close: > + close(fd); > +destroy_memcg: > cg_destroy(memcg); > +free_memcg: > free(memcg); > - close(fd); > > return ret; > } Nit: keep the cleanup_* naming for labels to make it obvious and to be consistent with the rest of the file (e.g. cleanup_free, cleanup_memcg, cleanup_file/cleanup_all). See test_memcg_subtree_control(). I would honestly have one label to cleanup the memcg. Calling cg_destroy() on a non-existent memcg should be fine. rmdir() will just fail silently. All other tests do this and it's easier to read when we have fewer return paths. My advice would be cleanup_file and cleanup_memcg labels. Thanks! With these nits: Reviewed-by: Yosry Ahmed > -- > 2.35.1 >