linux-kernel - Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1511251834.4822.113.camel@oracle.com>
Date:   Tue, 21 Nov 2017 09:10:34 +0100
From:   Knut Omang <knut.omang@...cle.com>
To:     Jim Davis <jim.epost@...il.com>,
        Luc Van Oostenryck <luc.vanoostenryck@...il.com>
Cc:     Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Michal Marek <mmarek@...e.com>,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        Andy Whitcroft <apw@...onical.com>,
        Joe Perches <joe@...ches.com>
Subject: Re: [PATCH 2/7] kbuild: Add P= command line flag to run checkpatch

On Mon, 2017-11-20 at 17:00 -0700, Jim Davis wrote:
> On Mon, Nov 20, 2017 at 2:22 PM, Luc Van Oostenryck
> <luc.vanoostenryck@...il.com> wrote:
> 
> > Should it be possible to somehow keep the distinction between
> > the flags coming from KBUILD_CFLAGS and the pure CHECKFLAGS?
> 
> Well, the practical problem seems to be that $(CHECK) is called in
> scripts/Makefile.build with both $(CHECKFLAGS) and $(c_flags) as
> arguments, regardless of what $(CHECK) is.  That can be hacked around
> with something inelegant like
> 
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index bb831d49bcfd..102194f9afb9 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -98,14 +98,20 @@ __build: $(if $(KBUILD_BUILTIN),$(builtin-target)
> $(lib-target) $(extra-y)) \
>          $(subdir-ym) $(always)
>         @:
> 
> -# Linus' kernel sanity checking tool
> +# Linus' kernel sanity checking tool, or $CHECK program of choice
> +ifneq ($(KBUILD_CHECKSRC),)
> +  add_to_checkflags =
> +  ifeq ($(CHECK),sparse)
> +    add_to_checkflags = $(c_flags)
> +  endif
> +endif
>  ifneq ($(KBUILD_CHECKSRC),0)
>    ifeq ($(KBUILD_CHECKSRC),2)
>      quiet_cmd_force_checksrc = CHECK   $<
> -          cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
> +          cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(add_to_checkflags) $<
> ;
>    else
>        quiet_cmd_checksrc     = CHECK   $<
> -            cmd_checksrc     = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
> +            cmd_checksrc     = $(CHECK) $(CHECKFLAGS) $(add_to_checkflags) $<
> ;
>    endif
>  endif
> 
> and then if I run scripts/checkpatch.pl as $CHECK and pass --quiet
> --file as before it works -- until checkpatch returns with a non-zero
> exit code, which causes the Makefile to bail at that point.

yes, in an earlier version, I added a --no-errors flag to checkpatch to handle
that, but based on feedback this version promotes make -k instead. It seems
however that that only holds to the next linker step. It is useful to be able to
select whether checkpatch should cause make to stop or not. While fixing issues,
failure is a better strategy while to assess the state or generate a report, a
return 0 behavior is better.

> So maybe a wrapper script, with an exit 0 fixup to keep on keeping on
> in case of checkpatch warnings or errors, would be better after all.

I can prepare a v2 based on the wrapper script I have already.

In that version, all added functionality was implemented in the wrapper
(including the configuration file parsing) 

Would you like to keep the checkpatch changes in some form, or would you rather
see everything happening in the wrapper?

A 3rd possibility that strikes me is that maybe what's needed is a general
checker runner that can also take care of running other checks, running multiple
checks, and maybe also handling temporary or decided suppression of sparse
errors in a similar fashion as I implemented for checkpatch errors. But maybe
that can be left to a later change set..

Thanks,
Knut