Re: [PATCH] checkpatch: Detect '%#' or '%0#' in printf-style format strings

[Dov Murik]

On 14/09/2020 9:55, Philippe Mathieu-Daudé wrote:
> +qemu-perl team
>
> On 9/14/20 8:01 AM, Dov Murik wrote:
> > According to the coding style document, we should use literal '0x' prefix
> > instead of printf's '#' flag (which appears as '%#' or '%0#' in the format
> > string).  Add a checkpatch rule to enforce that.
> >
> > Note that checkpatch already had a similar rule for trace-events files.
> >
> > Example usage:
> >
> >    $ scripts/checkpatch.pl --file chardev/baum.c
> >    ...
> >    ERROR: Don't use '#' flag of printf format ('%#') in format strings, use 
> > '0x' prefix instead
> >    #366: FILE: chardev/baum.c:366:
> >    +            DPRINTF("Broken packet %#2x, tossing\n", req); \
> >    ...
> >    ERROR: Don't use '#' flag of printf format ('%#') in format strings, use 
> > '0x' prefix instead
> >    #472: FILE: chardev/baum.c:472:
> >    +        DPRINTF("unrecognized request %0#2x\n", req);
> >    ...
> >
> > Signed-off-by: Dov Murik <dovmurik@linux.vnet.ibm.com>
> > ---
> >   scripts/checkpatch.pl | 12 ++++++++++++
> >   1 file changed, 12 insertions(+)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index bd3faa154c..6ec2a9f6a1 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2891,6 +2891,18 @@ sub process {
> >                         }
> >                 }
> >   
> > +# check for %# or %0# in printf-style format strings
> > +               while ($line =~ /(?:^|")([X\t]*)(?:"|$)/g) {
> > +                       my $string = substr($rawline, $-[1], $+[1] - $-[1]);
> > +                       $string =~ s/%%/__/g;
> > +                       if ($string =~ /(?<!%)%0?#/) {
> > +                               ERROR("Don't use '#' flag of printf format " .
> > +                                     "('%#') in format strings, use '0x' " .
> > +                                     "prefix instead\n" . $herecurr);
> > +                               last;
> > +                       }
> > +               }
> > +
> >   # QEMU specific tests
> >                 if ($rawline =~ /\b(?:Qemu|QEmu)\b/) {
> >                         ERROR("use QEMU instead of Qemu or QEmu\n" . $herecurr);
>
> Thank you for this patch!
>
> What about folding it in the same block?

That makes sense, except that 'last' statement which will escape the 
loop if one of the bad patterns is found.

Maybe we can just drop 'last' from both if-then blocks?  We'll get 
multiple alerts if bad patterns are used more than once in the same 
line, which sounds OK to me.


> -- >8 --
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2880,15 +2880,22 @@ sub process {
>                                  $herecurr);
>                  }
>
> -# check for %L{u,d,i} in strings
> +# format strings checks
>                  my $string;
>                  while ($line =~ /(?:^|")([X\t]*)(?:"|$)/g) {
>                          $string = substr($rawline, $-[1], $+[1] - $-[1]);
>                          $string =~ s/%%/__/g;
> +                       # check for %L{u,d,i} in strings
>                          if ($string =~ /(?<!%)%L[udi]/) {
>                                  ERROR("\%Ld/%Lu are not-standard C, use
> %lld/%llu\n" . $herecurr);
>                                  last;
>                          }
> +                       if ($string =~ /(?<!%)%0?#/) {
> +                               ERROR("Don't use '#' flag of printf
> format " .
> +                                     "('%#') in format strings, use
> '0x' " .
> +                                     "prefix instead\n" . $herecurr);
> +                               last;
> +                       }
>                  }
>
>   # QEMU specific tests
> ---