linux-kernel - Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

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: <CABVgOSkKCXodwi=RcmRpB+t157surmEjq2b+92VQQD2Cy0WTvA@mail.gmail.com>
Date:   Thu, 31 Oct 2019 00:12:22 -0700
From:   David Gow <davidgow@...gle.com>
To:     Joe Perches <joe@...ches.com>
Cc:     Dan Carpenter <dan.carpenter@...cle.com>, shuah <shuah@...nel.org>,
        Brendan Higgins <brendanhiggins@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>, kunit-dev@...glegroups.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the
 'list' doubly linked list

I tend to agree that it's better to either fix or ignore checkpatch
than to arbitrarily change things in cases like this where checkpatch
is obviously wrong. Equally, it certainly seems that there isn't an
obvious way of modifying checkpatch that will both not cause other
problems and not add another arbitrary name check. The main concern
about just leaving the checkpatch errors in is that people might be
automatically rejecting changes (or worse, the whole kselftest/test
pull request) if checkpatch errors are present. I'm not sure how
likely that is, but I can understand the desire to be careful, since
relatively minor changes have delayed KUnit changes before.

So, there are a few options, I guess:
- Hack around the issue in the patch (as this v7 is doing). Ugly, but
does at least mean that this change won't trigger any automated
rejection-of-checkpatch-errors people might be doing. (Even if, I
think we agree, automatically rejecting anything with checkpatch
warnings is not really correct.)
- Accept that tests (and other functions) with "for_each" in the name
like this are rare enough that it's not worth the complexity of
supporting it in checkpatch, and taking v6 as-is with the checkpatch
errors.
- Modify checkpatch to handle this in some other way (e.g., only if
the name doesn't include "test"): I don't think there's a perfectly
clean way of doing this.
- Modify checkpatch to make this ERROR a WARNING instead, since we
know this check has some flaws in this test, and potentially future
tests.
- Re-send v6 with a note about the checkpatch warning in the
description, so that it's easier to tell if one or more of these

Is there some combination of the above that sounds good?

-- David