Re: [PATCH next v2 2/3] checkpatch: add ethtool_sprintf rules
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.
Re: [PATCH next v2 2/3] checkpatch: add ethtool_sprintf rules
On Thu, Oct 26, 2023 at 3:39 PM Joe Perches wrote:
>
> On Thu, 2023-10-26 at 21:56 +, 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.ca...@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
Re: [PATCH next v2 2/3] checkpatch: add ethtool_sprintf rules
On Thu, 2023-10-26 at 21:56 +, 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.ca...@perches.com/
> + }
> + }
> +
> +
> # typecasts on min/max could be min_t/max_t
> if ($perl_version_ok &&
> defined $stat &&
>
Re: [PATCH next v2 2/3] checkpatch: add ethtool_sprintf rules
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.ca...@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.
Re: [PATCH next v2 2/3] checkpatch: add ethtool_sprintf rules
On Thu, Oct 26, 2023 at 3:12 PM Vladimir Oltean wrote:
>
> On Thu, Oct 26, 2023 at 09:56:08PM +, 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(, driver[i].name);
> > or
> > 2) Use ethtool_sprintf() with a standalone "%s" fmt string:
> > | ethtool_sprintf(, "%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
> > ---
> > 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.ca...@perches.com/
Thanks
Justin
Re: [PATCH next v2 2/3] checkpatch: add ethtool_sprintf rules
On Thu, Oct 26, 2023 at 09:56:08PM +, 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(, driver[i].name);
> or
> 2) Use ethtool_sprintf() with a standalone "%s" fmt string:
> | ethtool_sprintf(, "%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
> ---
> 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.
