(Robo Bot?, I think he prefers Andy)
On Fri, 2016-02-05 at 22:58 -0800, Kevin Wern wrote:
> On Fri, Feb 5, 2016 at 1:46 AM, Dan Carpenter wrote:
> > On Fri, Feb 05, 2016 at 12:29:52AM -0800, Kevin Wern wrote:
> > > There is a section of checkpatch.pl that intends to ensure the return
> > > value of sscanf is always checked. However, in certain cases, like in
> > > drivers/staging/dgnc/dgnc.mod.c
> >
> > You shouldn't be running checkpatch.pl on a mod.c file. Those are just
> > a build artifact and not code.
> >
> > regards,
> > dan carpenter
>
> Whoops, guess I'm being a bit of a newb there. Regardless, isn't it still more
> precise to narrow this conditional down to function calls, rather than just
> the symbol itself?
>
> I can eliminate the bad use case from the commit.
>
> On Fri, Feb 5, 2016 at 9:26 PM, Joe Perches <joe@xxxxxxxxxxx> wrote:
> > On Fri, 2016-02-05 at 00:29 -0800, Kevin Wern wrote:
> > > There is a section of checkpatch.pl that intends to ensure the return
> > > value of sscanf is always checked. However, in certain cases, like in
> > > drivers/staging/dgnc/dgnc.mod.c, the symbol for sscanf is used without
> > > calling the function:
> > >
> > > static const struct modversion_info ____versions[]
> > > __used
> > > __attribute__((section("__versions"))) = {
> > > ...
> > > { 0x20c55ae0, __VMLINUX_SYMBOL_STR(sscanf) },
> > > ...
> > > };
> > >
> > > This currently results in a warning, which is undesirable. We should
> > > adjust the script's first regex condition to match *calls* to sscanf,
> > > not just the symbol itself.
> > >
> > > Signed-off-by: Kevin Wern <kevin.m.wern@xxxxxxxxx>
> > > ---
> > > scripts/checkpatch.pl | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > index 0147c91..199247d 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -5452,7 +5452,7 @@ sub process {
> > > # check for naked sscanf
> > > if ($^V && $^V ge 5.10.0 &&
> > > defined $stat &&
> > > - $line =~ /\bsscanf\b/ &&
> > > + $line =~ /\bsscanf\b\s*$balanced_parens/ &&
> >
> > No, that won't work. That's what the $stat line is for
> >
> > I suppose it could be /\bsscanf\s*\(/ though
>
> I thought the $stat line's purpose was to ensure a statement was being
> evaluated in the current context. I tested the change with the example
> statement, and it both eliminated the original example and retained the
> ability to recognize unchecked sscanf return values. Although the example
> statement is from an artifact that shouldn't be checked by the script, it is
> still a valid example.
It's a single line sscanf vs multi-line sscanf issue
$line works on
ret = sscanf(buf, &foo, &bar);
$stat works on that and
ret = sscanf(buf,
&foo,
&bar);
>
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
