Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
MIME-Version: 1.0
References: <20231025-ethtool_puts_impl-v1-0-6a53a93d3b72@google.com>
 <20231025-ethtool_puts_impl-v1-2-6a53a93d3b72@google.com> <10a072f549e187bc2fdc735c0161c09c90fc1392.camel@perches.com>
In-Reply-To: <10a072f549e187bc2fdc735c0161c09c90fc1392.camel@perches.com>
From:   Justin Stitt <justinstitt@google.com>
Date:   Wed, 25 Oct 2023 16:59:10 -0700
Message-ID: <CAFhGd8r5YFJrpy7xvhi2LZUrsPNTTpWKy2PYgDOjnrnTNBN3Bg@mail.gmail.com>
Subject: Re: [PATCH 2/3] treewide: Convert some ethtool_sprintf() to ethtool_puts()
To:     Joe Perches <joe@perches.com>
Cc:     "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>,
        Dwaipayan Ray <dwaipayanray1@gmail.com>,
        Lukas Bulwahn <lukas.bulwahn@gmail.com>,
        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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Oct 25, 2023 at 4:51=E2=80=AFPM Joe Perches <joe@perches.com> wrote=
:
>
> On Wed, 2023-10-25 at 23:40 +0000, Justin Stitt wrote:
> > This patch converts some basic cases of ethtool_sprintf() to
> > ethtool_puts().
> >
> > The conversions are used in cases where ethtool_sprintf() was being use=
d
> > with just two arguments:
> > >       ethtool_sprintf(&data, buffer[i].name);
>
> OK.
>
> > or when it's used with format string: "%s"
> > >       ethtool_sprintf(&data, "%s", buffer[i].name);
> > > which both now become:
> > >       ethtool_puts(&data, buffer[i].name);
>
> Why do you want this conversion?
> Is it not possible for .name to contain a formatting field?

The case of using just two arguments to a ethtool_sprintf
call may cause -Wformat-security warnings. If it did indeed
have format specifiers then we would have more format
specifiers than arguments. Not ideal.

The second case of having a standalone "%s" isn't
necessarily bad or wrong. I used this exact approach to
replace some strncpy() usage in net drivers [1].

I'm working off guidance from Andrew Lunn [2] and Kees
who said it may be a good idea to tidy this up with a puts().

All in all, this patch doesn't do much but fix some warnings
and provide a more obvious interface. The number of
actual replacements are relatively low (around 20ish) so
I was hoping to sneak them in via this series.

>

[1]: https://lore.kernel.org/all/?q=3Ddfb%3Aethtool_sprintf+AND+f%3Ajustins=
titt
[2]: https://lore.kernel.org/all/a958d35e-98b6-4a95-b505-776482d1150c@lunn.=
ch/

Thanks
Justin