netdev - Re: [PATCH 2/6] list: add new MACROs to make iterator invisiable outside the loop

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAHk-=whJX52b1jNsmzXeVr6Z898R=9rBcSYx2oLt69XKDbqhOg@mail.gmail.com>
Date:   Thu, 3 Mar 2022 12:02:14 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Xiaomeng Tong <xiam0nd.tong@...il.com>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Jakob Koschel <jakobkoschel@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Kees Cook <keescook@...omium.org>,
        Jann Horn <jannh@...gle.com>,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>, Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH 2/6] list: add new MACROs to make iterator invisiable
 outside the loop

On Mon, Feb 28, 2022 at 11:59 PM Xiaomeng Tong <xiam0nd.tong@...il.com> wrote:
>
> +#define list_for_each_entry_inside(pos, type, head, member)            \

So as mentioned in another thread, I actually tried exactly this.

And it was horrendous.

It's _technically_ probably a very nice solution, but

 - it means that the already *good* cases are the ones that are
penalized by having to change

 - the syntax of the thing becomes absolutely nasty

which means that _practially_ it's exactly the wrong thing to do.

Just as an example, this is a random current "good user" in kernel/exit.c:

-       list_for_each_entry_safe(p, n, dead, ptrace_entry) {
+       list_for_each_entry_safe_inside(p, n, struct task_struct,
dead, ptrace_entry) {

and while some of the effects are nice (no need to declare p/n ahead
of time), just look at how nasty that line is.

Basically every single use will result in an over-long line. The above
example has minimal indentation, almost minimal variable names (to the
point of not being very descriptive at all), and one of the most basic
kernel structure types. And it still ended up 87 columns wide.

 And no, the answer to that is not "do it on multiple lines then".
That is just even worse.

So I really think this is a major step in the wrong direction.

We should strive for the *bad* cases to have to do extra work, and
even there we should really strive for legibility.

Now, I think that "safe" version in particular can be simplified:
there's no reason to give the "n" variable a name. Now that we can
(with -stc=gnu11) just declare our own variables in the for-loop, the
need for that externally visible 'next' declaration just goes away.

So three of those 87 columns are pointless and should be removed. The
macro can just internally decare 'n' like it always wanted (but
couldn't do due to legacy C language syntax restrictions).

But even with that fixed, it's still a very cumbersome line.

Note how the old syntax was "only" 60 characters - long but still
quite legible (and would have space for two more levels of indentation
without even hitting 80 characters). And that was _despute_ having to
have that 'n' declaration.

And yes, the old syntax does require that

        struct task_struct *p, *n;

line to declare the types, but that really is not a huge burden, and
is not complicated. It's just another "variables of the right type"
line (and as mentioned, the 'n' part has always been a C syntax
annoyance).

              Linus