linux-kernel - Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

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]

Date:   Thu, 22 Sep 2016 13:43:42 +0300
From:   Jani Nikula <jani.nikula@...ux.intel.com>
To:     Jean Delvare <jdelvare@...e.de>, Joe Perches <joe@...ches.com>
Cc:     Julia Lawall <julia.lawall@...6.fr>,
        Al Viro <viro@...IV.linux.org.uk>,
        Ilya Dryomov <idryomov@...il.com>,
        Andy Whitcroft <apw@...onical.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Jonathan Corbet <corbet@....net>,
        Ceph Development <ceph-devel@...r.kernel.org>,
        Alex Elder <elder@...nel.org>, Sage Weil <sage@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        kernel-janitors@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-doc@...r.kernel.org
Subject: Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

On Thu, 22 Sep 2016, Jean Delvare <jdelvare@...e.de> wrote:
> Hi Joe,
>
> On Mon, 19 Sep 2016 23:32:03 -0700, Joe Perches wrote:
>> On Tue, 2016-09-20 at 07:53 +0200, Julia Lawall wrote:
>> > I think it is better to be clear.  CHECK was never really clear to me,
>> > especially if you see it in isolation, on a file that doesn't also have
>> > ERROR or WARNING.  NITS is a common word in this context, but not FLEAS
>> > and GNATS, as far as I know.
>> > There could also be a severity level: high medium and low
>> 
>> I agree clarity is good.
>> 
>> The seriousness with which some beginners take these message
>> types though is troublesome,
>
> You need to think in terms of actual use cases. Who uses checkpatch and
> why? I think there are 3 groups of users:
> * Beginners. They won't run the script by themselves, instead they will
>   submit a patch which infringes a lot of coding style rules, and the
>   maintainer will point them to checkpatch and ask for a resubmission
>   which makes checkpatch happy. Being beginners, they can only rely on
>   the script itself to only report things which need to be fixed, by
>   default.
> * Experienced developers. Who simply want to make sure they did not
>   overlook anything before they post their work for review. They have
>   the knowledge to decide if they want to ignore some of the warnings.
> * People with too much spare time, looking for anything they could
>   "contribute" to the kernel. They will use --subjective and piss off
>   every maintainer they can find.
>
> Sadly there's not much we can do about the third category, short of
> killing option --subjective altogether.

You could make checkpatch have different defaults for patches and files,
to encourage better style in new code, but to discourage finding
problems in existing code.

BR,
Jani.


>
> The default settings should work out of the box for for the first
> category.
>
>> Maybe prefix various different types of style messages.
>> 
>> Something like:
>> 
>> ERROR	-> CODE_STYLE_DEFECT
>> WARNING -> CODE_STYLE_UNPREFERRED
>> CHECK	-> CODE_STYLE_NIT
>
> I don't think you clarify anything with these changes, as they do not
> carry the requirement from a submitter's perspective. If you really
> want to change the names, I would rather suggest:
>
> ERROR -> MUST_FIX
> WARNING -> SHOULD_FIX
> CHECK -> MAY_FIX
>
> Or explain the categories in plain English, see below.
>
>> I doubt additional external documentation would help much.
>
> I agree. If anything needs to be explained, it should be included in
> the output of checkpatch directly. For example, if any ERROR was
> printed, include the following after the count summary:
>
> "ERROR means the code is infringing a core coding style rule. This MUST
> be fixed before the patch is submitted upstream."
>
> And if any WARNING was printed, include the following:
>
> "WARNING means the code is not following the best practice. This SHOULD
> be fixed before the patch is submitted upstream, unless the maintainer
> agreed otherwise."
>
> Not sure if we need something for CHECK, as these messages are not
> printed by default so I'd assume people who get them know what they
> asked for. But apparently these confused Julia so maybe a similar
> explanation would be needed for them too.

-- 
Jani Nikula, Intel Open Source Technology Center