Message ID | 20231026-ethtool_puts_impl-v2-2-0d67cbdd0538@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:d641:0:b0:403:3b70:6f57 with SMTP id cy1csp192663vqb; Thu, 26 Oct 2023 14:57:35 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEM/r+Gip9A5fwd/ylw3nh847tfFWxY6YivuumoiV4E1EJS3sHGeL8C9XYNBwTpv8coqMME X-Received: by 2002:ac2:47fb:0:b0:507:b8e1:76f0 with SMTP id b27-20020ac247fb000000b00507b8e176f0mr486338lfp.22.1698357454913; Thu, 26 Oct 2023 14:57:34 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698357454; cv=none; d=google.com; s=arc-20160816; b=DqabWHMAYm2BB9q8PVS2j93JhrNEImyDqsSRodCqooIPkg+B3LWUS2cp/vPowSfzxp AFD0w7iIMPsaP0/jjfN26O5rqGvncSBfCsrDZK2/ymupQDXTt4kcV/xRqsC6Cs3bi/xp gB8mk69j7QiDE+2WIRP5p9YKGDX4RyyPyvID4idy5u2GXBwLkTgvgwFfYGzDih8Xsw/Q fJQA712zHtMnvrJwDxcvrq720NR9pY0ZzdIk/JiHI2ZXCuuUNJXbz6KkwTa+DK8VKl11 0QfZ6ofTu5n49lk2fN0eZ+COu6E8jkTS7XWjf/567CILDAfo/Gyn+nQM4mCajvTSoEtP iVJw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:dkim-signature; bh=dYFCbbBAM6Yzga/rYjukRYsp/POsV5oivqv/XEKMawY=; fh=KbtZdS+g7EcIXF6IkhyVCA7RdLprHttm0sN0x6fLY0g=; b=l62Oxc7T6hp104AqHw3QhJT23ba2t+cnzDUq0JycB7GNn8NZafaPT2dTHKz1F1EqO5 /OKzc8oBroaWWGaXFIMsv6j46k8i+gpavQVzAK1TwofIT/c/HVbKRbV3Ssfza/MilRoP 1jQ6chWWX8gHPZaTVTLXrut4IbbkSiX/fU75e0fAvaCU5nv89EIcyZAomFOCPIissAbr tFFon3a7CkatGXUexhZ8Hn5Z15gOxNnIl0HSakyWMQ2FvYkHMLdfUa6f3yLlwSIZ/TYh DXfoYgnakd+Cu+puXqNP340J9L5CMTP3K8trxzf1yFACy4Dd/Lp+HXd86xzC5Gpu2Q30 +I8w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=s3uedDKO; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from groat.vger.email (groat.vger.email. [23.128.96.35]) by mx.google.com with ESMTPS id r20-20020a05640251d400b0053d9f0faa0csi149586edd.393.2023.10.26.14.57.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 26 Oct 2023 14:57:34 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=s3uedDKO; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by groat.vger.email (Postfix) with ESMTP id 7232D80253A6; Thu, 26 Oct 2023 14:57:08 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at groat.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1344897AbjJZV42 (ORCPT <rfc822;aposhian.dev@gmail.com> + 26 others); Thu, 26 Oct 2023 17:56:28 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54030 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232068AbjJZV4U (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 26 Oct 2023 17:56:20 -0400 Received: from mail-yb1-xb4a.google.com (mail-yb1-xb4a.google.com [IPv6:2607:f8b0:4864:20::b4a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 196431B6 for <linux-kernel@vger.kernel.org>; Thu, 26 Oct 2023 14:56:17 -0700 (PDT) Received: by mail-yb1-xb4a.google.com with SMTP id 3f1490d57ef6-d815354ea7fso1013636276.1 for <linux-kernel@vger.kernel.org>; Thu, 26 Oct 2023 14:56:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1698357376; x=1698962176; darn=vger.kernel.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=dYFCbbBAM6Yzga/rYjukRYsp/POsV5oivqv/XEKMawY=; b=s3uedDKO57Gbbjj5ckqs2yI/l/NMPrzoFUTVteFYNqT0wdbrNFOyCbCJuFEByoxTAA JQpgp42ryiIzTlXUHumVLjTW3WjN90jmHeBHezePUDDhRMm2fK4MZDnLHu25esuq/idr eyFwZhEC6sl7ZaIOcK0Rv0eAzryGQPveVBbB3/uO5m9fGNNItWJebKyErRLQYUTXIFyr 9iQwGzpsdUfkwLm2UdN/pYlmE3LgUVCmWK+vVUIESVZHzub3JTFXIy65UBQ4xeonC2Cw bThaKeZSTSA+F9yZrBQSDUMoeACAken3LAGWVDOThmuvzJ+IvnrO9zGzLB/GSf5jyDoX mcAA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698357376; x=1698962176; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=dYFCbbBAM6Yzga/rYjukRYsp/POsV5oivqv/XEKMawY=; b=Zvh752tYaKgPTuGnbGq9OvWX1XT1dFFiBFAQatCceO7YzPR+BE4cgiRxO8JR3GAbeS Eu0NchyWf/NncFjszEhUoPBCPxlgeEjMxEfFn/84hgvIDTV0BDe0Kz6sfvGS2w9vJ9Js p8501bYc4bMoa2s79TKZWqOU9UFjCQdZTXTKTPKFwBuhI5fQuuI0HrsGhiqC4NJ+FzAx hN5aGPKi/2qZWLuV8zO8Yxt8FyP/l2niXdm1P2vbM/+HmMKjmPBfycpGy2aC/56p00xc YbLdCmZ119UB2PksPUuTHb/Sffu9BK5LjqNV6MaQ9z45X227nFIQ+EtL+3PJq97nWNma QR3A== X-Gm-Message-State: AOJu0YwsKUx5IntTYtySIhoufeIUCVQ9VPQZcOvZo2NrrX8MUy9gtlcd 21lwrE6NwlfZBPf1HFFr2BNNVbk4iCV73GuI1A== X-Received: from jstitt-linux1.c.googlers.com ([fda3:e722:ac3:cc00:2b:ff92:c0a8:23b5]) (user=justinstitt job=sendgmr) by 2002:a5b:9d2:0:b0:d9a:bce6:acf3 with SMTP id y18-20020a5b09d2000000b00d9abce6acf3mr18000ybq.0.1698357376212; Thu, 26 Oct 2023 14:56:16 -0700 (PDT) Date: Thu, 26 Oct 2023 21:56:08 +0000 In-Reply-To: <20231026-ethtool_puts_impl-v2-0-0d67cbdd0538@google.com> Mime-Version: 1.0 References: <20231026-ethtool_puts_impl-v2-0-0d67cbdd0538@google.com> X-Developer-Key: i=justinstitt@google.com; a=ed25519; pk=tC3hNkJQTpNX/gLKxTNQKDmiQl6QjBNCGKJINqAdJsE= X-Developer-Signature: v=1; a=ed25519-sha256; t=1698357372; l=1942; i=justinstitt@google.com; s=20230717; h=from:subject:message-id; bh=Tttx+U2qjudejsJUauC/KpxZsIK2OKqCPCc+Ch/fVls=; b=elpAQEsU3HOMMos/TnCJkrVo+AFG/1lerU81cOH+T2BgSHRjrFvGR7BmTA25L2uIiNK2WLsQZ m5JhXI8wJOxDHnWxVC+YAmf+U1BnkfHv0NAyeBvW/Fv+InbmfMY7Pw4 X-Mailer: b4 0.12.3 Message-ID: <20231026-ethtool_puts_impl-v2-2-0d67cbdd0538@google.com> Subject: [PATCH next v2 2/3] checkpatch: add ethtool_sprintf rules From: Justin Stitt <justinstitt@google.com> To: "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Shay Agroskin <shayagr@amazon.com>, Arthur Kiyanovski <akiyano@amazon.com>, David Arinzon <darinzon@amazon.com>, Noam Dagan <ndagan@amazon.com>, Saeed Bishara <saeedb@amazon.com>, Rasesh Mody <rmody@marvell.com>, Sudarsana Kalluru <skalluru@marvell.com>, GR-Linux-NIC-Dev@marvell.com, Dimitris Michailidis <dmichail@fungible.com>, Yisen Zhuang <yisen.zhuang@huawei.com>, Salil Mehta <salil.mehta@huawei.com>, Jesse Brandeburg <jesse.brandeburg@intel.com>, Tony Nguyen <anthony.l.nguyen@intel.com>, Louis Peens <louis.peens@corigine.com>, Shannon Nelson <shannon.nelson@amd.com>, Brett Creeley <brett.creeley@amd.com>, drivers@pensando.io, "K. Y. Srinivasan" <kys@microsoft.com>, Haiyang Zhang <haiyangz@microsoft.com>, Wei Liu <wei.liu@kernel.org>, Dexuan Cui <decui@microsoft.com>, Ronak Doshi <doshir@vmware.com>, VMware PV-Drivers Reviewers <pv-drivers@vmware.com>, Andy Whitcroft <apw@canonical.com>, Joe Perches <joe@perches.com>, Dwaipayan Ray <dwaipayanray1@gmail.com>, Lukas Bulwahn <lukas.bulwahn@gmail.com>, Hauke Mehrtens <hauke@hauke-m.de>, Andrew Lunn <andrew@lunn.ch>, Florian Fainelli <f.fainelli@gmail.com>, Vladimir Oltean <olteanv@gmail.com>, " =?utf-8?b?QXLEsW7DpyDDnE5BTA==?= " <arinc.unal@arinc9.com>, Daniel Golle <daniel@makrotopia.org>, Landen Chao <Landen.Chao@mediatek.com>, DENG Qingfang <dqfext@gmail.com>, Sean Wang <sean.wang@mediatek.com>, Matthias Brugger <matthias.bgg@gmail.com>, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>, Linus Walleij <linus.walleij@linaro.org>, " =?utf-8?q?Alvin_=C5=A0ipraga?= " <alsi@bang-olufsen.dk>, Wei Fang <wei.fang@nxp.com>, Shenwei Wang <shenwei.wang@nxp.com>, Clark Wang <xiaoning.wang@nxp.com>, NXP Linux Team <linux-imx@nxp.com>, Lars Povlsen <lars.povlsen@microchip.com>, Steen Hegelund <Steen.Hegelund@microchip.com>, Daniel Machon <daniel.machon@microchip.com>, UNGLinuxDriver@microchip.com, Jiawen Wu <jiawenwu@trustnetic.com>, Mengyuan Lou <mengyuanlou@net-swift.com>, Heiner Kallweit <hkallweit1@gmail.com>, Russell King <linux@armlinux.org.uk>, Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, Jesper Dangaard Brouer <hawk@kernel.org>, John Fastabend <john.fastabend@gmail.com> Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org, Nick Desaulniers <ndesaulniers@google.com>, Nathan Chancellor <nathan@kernel.org>, Kees Cook <keescook@chromium.org>, intel-wired-lan@lists.osuosl.org, oss-drivers@corigine.com, linux-hyperv@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, bpf@vger.kernel.org, Justin Stitt <justinstitt@google.com> Content-Type: text/plain; charset="utf-8" X-Spam-Status: No, score=-8.4 required=5.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Thu, 26 Oct 2023 14:57:08 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1780856867084768429 X-GMAIL-MSGID: 1780856867084768429 |
Series |
ethtool: Add ethtool_puts()
|
|
Commit Message
Justin Stitt
Oct. 26, 2023, 9:56 p.m. UTC
Add some warnings for using ethtool_sprintf() where a simple
ethtool_puts() would suffice.
The two cases are:
1) Use ethtool_sprintf() with just two arguments:
| ethtool_sprintf(&data, driver[i].name);
or
2) Use ethtool_sprintf() with a standalone "%s" fmt string:
| ethtool_sprintf(&data, "%s", driver[i].name);
The former may cause -Wformat-security warnings while the latter is just
not preferred. Both are safely in the category of warnings, not errors.
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
scripts/checkpatch.pl | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
Comments
On Thu, Oct 26, 2023 at 09:56:08PM +0000, Justin Stitt wrote: > Add some warnings for using ethtool_sprintf() where a simple > ethtool_puts() would suffice. > > The two cases are: > > 1) Use ethtool_sprintf() with just two arguments: > | ethtool_sprintf(&data, driver[i].name); > or > 2) Use ethtool_sprintf() with a standalone "%s" fmt string: > | ethtool_sprintf(&data, "%s", driver[i].name); > > The former may cause -Wformat-security warnings while the latter is just > not preferred. Both are safely in the category of warnings, not errors. > > Signed-off-by: Justin Stitt <justinstitt@google.com> > --- > scripts/checkpatch.pl | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 25fdb7fda112..22f007131337 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -7011,6 +7011,25 @@ sub process { > "Prefer strscpy, strscpy_pad, or __nonstring over strncpy - see: https://github.com/KSPP/linux/issues/90\n" . $herecurr); > } > > +# ethtool_sprintf uses that should likely be ethtool_puts > + if ($line =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) { > + if(WARN("ETHTOOL_SPRINTF", > + "Prefer ethtool_puts over ethtool_sprintf with only two arguments\n" . $herecurr) && > + $fix) { > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(/ethtool_puts\(/; > + } > + } > + > + # use $rawline because $line loses %s via sanitization and thus we can't match against it. > + if ($rawline =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/) { > + if(WARN("ETHTOOL_SPRINTF", > + "Prefer ethtool_puts over ethtool_sprintf with standalone \"%s\" specifier\n" . $herecurr) && > + $fix) { > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(\s*(.*?),.*?,(.*?)\)/ethtool_puts\($1,$2)/; > + } > + } > + > + > # typecasts on min/max could be min_t/max_t > if ($perl_version_ok && > defined $stat && > > -- > 2.42.0.820.g83a721a137-goog > I don't really know Perl, but does the indentation and coding style here conform to any rules, or is it just free-form? The rest of the script looks almost as you'd expect from C. This is unreadable to me.
On Thu, Oct 26, 2023 at 3:12 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > On Thu, Oct 26, 2023 at 09:56:08PM +0000, Justin Stitt wrote: > > Add some warnings for using ethtool_sprintf() where a simple > > ethtool_puts() would suffice. > > > > The two cases are: > > > > 1) Use ethtool_sprintf() with just two arguments: > > | ethtool_sprintf(&data, driver[i].name); > > or > > 2) Use ethtool_sprintf() with a standalone "%s" fmt string: > > | ethtool_sprintf(&data, "%s", driver[i].name); > > > > The former may cause -Wformat-security warnings while the latter is just > > not preferred. Both are safely in the category of warnings, not errors. > > > > Signed-off-by: Justin Stitt <justinstitt@google.com> > > --- > > scripts/checkpatch.pl | 19 +++++++++++++++++++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > index 25fdb7fda112..22f007131337 100755 > > --- a/scripts/checkpatch.pl > > +++ b/scripts/checkpatch.pl > > @@ -7011,6 +7011,25 @@ sub process { > > "Prefer strscpy, strscpy_pad, or __nonstring over strncpy - see: https://github.com/KSPP/linux/issues/90\n" . $herecurr); > > } > > > > +# ethtool_sprintf uses that should likely be ethtool_puts > > + if ($line =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) { > > + if(WARN("ETHTOOL_SPRINTF", > > + "Prefer ethtool_puts over ethtool_sprintf with only two arguments\n" . $herecurr) && > > + $fix) { > > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(/ethtool_puts\(/; > > + } > > + } > > + > > + # use $rawline because $line loses %s via sanitization and thus we can't match against it. > > + if ($rawline =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/) { > > + if(WARN("ETHTOOL_SPRINTF", > > + "Prefer ethtool_puts over ethtool_sprintf with standalone \"%s\" specifier\n" . $herecurr) && > > + $fix) { > > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(\s*(.*?),.*?,(.*?)\)/ethtool_puts\($1,$2)/; > > + } > > + } > > + > > + > > # typecasts on min/max could be min_t/max_t > > if ($perl_version_ok && > > defined $stat && > > > > -- > > 2.42.0.820.g83a721a137-goog > > > > I don't really know Perl, but does the indentation and coding style here > conform to any rules, or is it just free-form? The rest of the script > looks almost as you'd expect from C. This is unreadable to me. There was some discussion here [1] but AFAICT I need to use EMACS or configure my vim in a very particular way to get the same formatting But yeah, look around line 7000 -- lots of this pattern matching code is pretty hard to read. Not sure there's much to be done as far as readability is concerned. [1]: https://lore.kernel.org/all/137a309b313cc8a295f3affc704f0da049f233aa.camel@perches.com/ Thanks Justin
On Thu, Oct 26, 2023 at 03:24:54PM -0700, Justin Stitt wrote: > There was some discussion here [1] but AFAICT I need to use EMACS > or configure my vim in a very particular way to get the same formatting > > But yeah, look around line 7000 -- lots of this pattern matching code is > pretty hard to read. Not sure there's much to be done as far as readability > is concerned. > > [1]: https://lore.kernel.org/all/137a309b313cc8a295f3affc704f0da049f233aa.camel@perches.com/ Hard to read because of pattern matching is one thing, but your indentation is unlike anything else in this file. There are inner curly brackets which are less indented than the outer curly brackets. I cannot read/review this, sorry, I hope somebody else can.
On Thu, 2023-10-26 at 21:56 +0000, Justin Stitt wrote: > Add some warnings for using ethtool_sprintf() where a simple > ethtool_puts() would suffice. [] > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > @@ -7011,6 +7011,25 @@ sub process { > "Prefer strscpy, strscpy_pad, or __nonstring over strncpy - see: https://github.com/KSPP/linux/issues/90\n" . $herecurr); > } > > +# ethtool_sprintf uses that should likely be ethtool_puts > + if ($line =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) { > + if(WARN("ETHTOOL_SPRINTF", > + "Prefer ethtool_puts over ethtool_sprintf with only two arguments\n" . $herecurr) && > + $fix) { > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(/ethtool_puts\(/; > + } > + } > + > + # use $rawline because $line loses %s via sanitization and thus we can't match against it. > + if ($rawline =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/) { > + if(WARN("ETHTOOL_SPRINTF", > + "Prefer ethtool_puts over ethtool_sprintf with standalone \"%s\" specifier\n" . $herecurr) && > + $fix) { > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(\s*(.*?),.*?,(.*?)\)/ethtool_puts\($1,$2)/; Thanks, but: This fix wouldn't work if the first argument was itself a function with multiple arguments. And this is whitespace formatted incorrectly. It: o needs a space after if o needs a space after the comma in the fix o needs to use the appropriate amount and tabs for indentation o needs alignment to open parentheses o the backslashes are not required in the fix block Do you want me to push what I wrote in the link below? https://lore.kernel.org/lkml/7eec92d9e72d28e7b5202f41b02a383eb28ddd26.camel@perches.com/ > + } > + } > + > + > # typecasts on min/max could be min_t/max_t > if ($perl_version_ok && > defined $stat && >
On Thu, Oct 26, 2023 at 3:39 PM Joe Perches <joe@perches.com> wrote: > > On Thu, 2023-10-26 at 21:56 +0000, Justin Stitt wrote: > > Add some warnings for using ethtool_sprintf() where a simple > > ethtool_puts() would suffice. > [] > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > [] > > @@ -7011,6 +7011,25 @@ sub process { > > "Prefer strscpy, strscpy_pad, or __nonstring over strncpy - see: https://github.com/KSPP/linux/issues/90\n" . $herecurr); > > } > > > > +# ethtool_sprintf uses that should likely be ethtool_puts > > + if ($line =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) { > > + if(WARN("ETHTOOL_SPRINTF", > > + "Prefer ethtool_puts over ethtool_sprintf with only two arguments\n" . $herecurr) && > > + $fix) { > > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(/ethtool_puts\(/; > > + } > > + } > > + > > + # use $rawline because $line loses %s via sanitization and thus we can't match against it. > > + if ($rawline =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/) { > > + if(WARN("ETHTOOL_SPRINTF", > > + "Prefer ethtool_puts over ethtool_sprintf with standalone \"%s\" specifier\n" . $herecurr) && > > + $fix) { > > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(\s*(.*?),.*?,(.*?)\)/ethtool_puts\($1,$2)/; > > Thanks, but: > > This fix wouldn't work if the first argument was itself a function > with multiple arguments. > > And this is whitespace formatted incorrectly. > > It: > > o needs a space after if > o needs a space after the comma in the fix > o needs to use the appropriate amount and tabs for indentation > o needs alignment to open parentheses > o the backslashes are not required in the fix block > > Do you want me to push what I wrote in the link below? > https://lore.kernel.org/lkml/7eec92d9e72d28e7b5202f41b02a383eb28ddd26.camel@perches.com/ Ah, I didn't see you provided a diff in previous thread. Yeah you can push it but it's not really a standalone so perhaps I'll just steal the diff and wrap into v3? > > > + } > > + } > > + > > + > > # typecasts on min/max could be min_t/max_t > > if ($perl_version_ok && > > defined $stat && > > > Thanks Justin
On 2023-10-27 12:40, Justin Stitt wrote: > Yeah you can push it but it's not really a standalone so perhaps I'll > just steal the diff and > wrap into v3? Fine by me. No need for my sign off.
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 25fdb7fda112..22f007131337 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -7011,6 +7011,25 @@ sub process { "Prefer strscpy, strscpy_pad, or __nonstring over strncpy - see: https://github.com/KSPP/linux/issues/90\n" . $herecurr); } +# ethtool_sprintf uses that should likely be ethtool_puts + if ($line =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) { + if(WARN("ETHTOOL_SPRINTF", + "Prefer ethtool_puts over ethtool_sprintf with only two arguments\n" . $herecurr) && + $fix) { + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(/ethtool_puts\(/; + } + } + + # use $rawline because $line loses %s via sanitization and thus we can't match against it. + if ($rawline =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/) { + if(WARN("ETHTOOL_SPRINTF", + "Prefer ethtool_puts over ethtool_sprintf with standalone \"%s\" specifier\n" . $herecurr) && + $fix) { + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(\s*(.*?),.*?,(.*?)\)/ethtool_puts\($1,$2)/; + } + } + + # typecasts on min/max could be min_t/max_t if ($perl_version_ok && defined $stat &&