On Thu, 03 Jun 2021 11:58:15 -0700 Joe Perches <joe@xxxxxxxxxxx> wrote: > On Thu, 2021-06-03 at 19:06 +0100, Jonathan Cameron wrote: > > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > > > > A wrong use of one of these in > > https://lore.kernel.org/linux-iio/20210514135927.2926482-1-arnd@xxxxxxxxxx/ > > included a reference from Nathan to a patch discouraging the use of > > these strings in general: > > https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q@xxxxxxxxxxxxxx/ > > > > I did a quick grep and established we only have a few instances of these in > > IIO anyway, so in the interests of avoiding those existing cases getting > > cut and paste into new drivers, let's just clear them out now. > > > > Note that patch from Arnd is now also part of this series, due to the > > length specifier related issue Joe raised. > > > > I have chosen to go with 0x%02x rather than %#04x as I find it more readable. > > > > V2: > > Use 0x%02x (Joe Perches) > > Include Arnd's original patch, modified for the above. > > Hello again. > > It looks to me as though %#<foo> is relatively commonly misused in the kernel. > > Pehaps for the decimal portion of the format, checkpatch could have some > test for use of non-standard lengths. > > Given the use is generally meant for a u8, u16, u32, or u64, perhaps > checkpatch should emit a warning whenever the length is not 4, 6, 10, or 18. Would have saved me some trouble, so I'm definitely in favour of checkpatch catching this. I wonder if a better option is to match on 1, 2, 4, 8, 16 as likely to be caused by people getting the usage wrong rather than a deliberate attempt to pretty print something a little unusual? Thanks. Jonathan > > (possible checkpatch patch below) > > $ git grep -P -h -o '%#\d+\w+' | sort | uniq -c | sort -rn > 392 %#08x > 238 %#04x > 144 %#02x > 114 %#06x > 92 %#010x > 58 %#010Lx > 55 %#018llx > 47 %#010llx > 45 %#010lx > 38 %#016llx > 27 %#0x > 23 %#2x > 18 %#016lx > 17 %#3lx > 17 %#08lx > 17 %#018Lx > 15 %#3x > 14 %#03x > 10 %#06hx > 9 %#08zx > 8 %#10x > 7 %#16llx > 6 %#8x > 6 %#04X > 6 %#04llx > 6 %#012llx > 5 %#16 > 4 %#08llx > 4 %#06llx > 4 %#05x > 4 %#02X > 4 %#016Lx > 3 %#04hx > 3 %#01x > 2 %#6x > 2 %#4x > 2 %#10 > 2 %#09x > 2 %#05lx > 1 %#8lx > 1 %#5x > 1 %#5lx > 1 %#2Lx > 1 %#2llx > 1 %#16x > 1 %#16lx > 1 %#12x > 1 %#0x10000 > 1 %#0lx > 1 %#08 > 1 %#05llx > 1 %#04o > 1 %#04lx > 1 %#03X > 1 %#018lx > 1 %#010zx > > --- > scripts/checkpatch.pl | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index d65334588eb4c..5840f3f2aee6f 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -6695,6 +6695,31 @@ sub process { > my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0)); > $fmt =~ s/%%//g; > > + while ($fmt =~ /\%#([\d]+)/g) { > + my $length = $1; > + my $pref_len; > + if ($length < 4) { > + $pref_len = '04'; > + } elsif ($length == 5) { > + $pref_len = '06'; > + } elsif ($length > 6 && $length < 10) { > + $pref_len = '010'; > + } elsif ($length > 10 && $length < 18) { > + $pref_len = '018'; > + } elsif ($length > 18) { > + $pref_len = '<something else>'; > + } > + if (defined($pref_len)) { > + if (!defined($stat_real)) { > + $stat_real = get_stat_real($linenr, $lc); > + } > + WARN("VSPRINTF_SPECIAL_LENGTH", > + "Unusual special length '%#$length' in 0x prefixed output, length is usually 2 more than the desired width - perhaps use '%#${pref_len}'\n" . "$here\n$stat_real"); > + } > + } > + > + pos($fmt) = 0; > + > while ($fmt =~ /(\%[\*\d\.]*p(\w)(\w*))/g) { > $specifier = $1; > $extension = $2; >
