Hi
2022. március 11., péntek 3:01 keltezéssel, Linus Torvalds írta:
> On Thu, Mar 10, 2022 at 5:42 PM Linus Torvalds
> <[email protected]> wrote:
> >
> > That one didn't do the automatic offset thing, but see
> >
> > https://lore.kernel.org/all/CAADWXX-Pr-D3wSr5wsqTEOBSJzB9k7bSH+7hnCAj0AeL0=U4mg@mail.gmail.com/
> >
> > on the problems that has.
>
> Note: I think the problems are serious enough that it almost certainly
> isn't worth doing - it makes the code uglier for very little upside.
>
> So I tried to explain how it _could_ be done, but that doesn't mean
> that it _should_ be done.
>
> Having the member name as part of the list traversal macro isn't
> actually generally a real problem.
>
> I added it to the list_traversal_head() macro in that original patch
> because I think we can easily use the member head name to _verify_
> that the declaration and the use match.
>
> Yes, squirrelling off the offset and not needing the member head name
> at all at when traversing the list is obviously simpler syntax, but
> that part has never been the real problem with list traversal. And
> verifying that the member name that is passed in is the same as in the
> list_traversal_head() would be trivial.
>
> To verify it, we could simply change that type name from:
>
> type *name##_traversal_type;
>
> to be
>
> type *name##_traversal_type_##member;
>
> instead, and suddenly the member name in 'list_traverse()' has to
> match that thing that list_traversal_head() created.
>
> So yes, you'd have that third argument in list_traverse(), but it
> would be trivially checked at compile-time.
That is indeed a simpler thing to do, and doesn't need `offsetof()` at the
declaration, but there are places - not many - where the `list_head` member
is inside a subobject, for example, so `member` now contains a period.
>
> And you'd avoid all the ugly complexities (described above) with lists
> that are embedded inside data structures that refer to each other)
Regards,
Barnabás Pőcze
On Thu, Mar 10, 2022 at 6:49 PM Barnabás Pőcze <[email protected]> wrote:
>
> That is indeed a simpler thing to do, and doesn't need `offsetof()` at the
> declaration, but there are places - not many - where the `list_head` member
> is inside a subobject, for example, so `member` now contains a period.
Ahh, very true. And very annoying. So close, yet so far, and no way I
can see to really deal with that.
And it's not even really all that rare. It may not be the _common_
case, but it's still fairly wide-spread and not some "one or two odd
places" thing.
This grep catches at least a subset of cases:
git grep '\<list_for_each_entry(.*\.[a-z_0-9]*)'
and it's clearly all over.
As mentioned, I don't think that we have had huge problems with
getting the member name wrong. We do get a fair amount of checking in
that it obviously has to be part of the type we iterate over, and even
if you were to pick the wrong one, the result is a very simple "that
doesn't work".
But it would still undeniably be very nice to have some automatic
build-time checking for it.
Now, some checking could be done by just doing the "reverse" of what
that patch in
https://lore.kernel.org/all/CAHk-=wiacQM76xec=Hr7cLchVZ8Mo9VDHmXRJzJ_EX4sOsApEA@mail.gmail.com/
does with 'list_traversal_head()', and have a
#define list_traversal_entry()
that has a similar union with a type that also specifies the list
entry type (with that same "append marker to member name" model, which
still works fine with dots in the middle).
Then at least cross-checking that the type of the iterator matches
with the target list member is trivial, but _if_ a type has multiple
list entries, they often end up referring to the same type.
That very patch with 'struct task_struct' converted is actually an
example of exactly that: the <children,sibling> and <ptraced,
ptrace_entry> pairings have the exact same type tuple (all being
'struct task_struct', of course).
So it would strengthen the typechecking a bit in some cases, but
probably not all that noticeably.
Linus
Hi
2022. március 11., péntek 6:06 keltezéssel, Linus Torvalds írta:
> On Thu, Mar 10, 2022 at 6:49 PM Barnabás Pőcze <[email protected]> wrote:
> >
> > That is indeed a simpler thing to do, and doesn't need `offsetof()` at the
> > declaration, but there are places - not many - where the `list_head` member
> > is inside a subobject, for example, so `member` now contains a period.
>
> Ahh, very true. And very annoying. So close, yet so far, and no way I
> can see to really deal with that.
>
> And it's not even really all that rare. It may not be the _common_
> case, but it's still fairly wide-spread and not some "one or two odd
> places" thing.
>
> This grep catches at least a subset of cases:
>
> git grep '\<list_for_each_entry(.*\.[a-z_0-9]*)'
>
> and it's clearly all over.
>
> As mentioned, I don't think that we have had huge problems with
> getting the member name wrong. We do get a fair amount of checking in
> that it obviously has to be part of the type we iterate over, and even
> if you were to pick the wrong one, the result is a very simple "that
> doesn't work".
>
> But it would still undeniably be very nice to have some automatic
> build-time checking for it.
>
Yes, I think compile-time errors are significantly better than whatever you get
at runtime. So, sorry to be this obtuse, but I gather the proposed interface
is a no-go in all ways, shapes, and forms in your eyes? I am fully aware that it
does not magically solve everything and it does not work in the "interesting"
and "convoluted" cases, but as mentioned in a previous email, I think there are
still a lot of "boring" places where it can be used. But of course I don't want
to bother anyone if it's a definitive no.
> [...]
Regards,
Barnabás Pőcze
On Fri, Mar 11, 2022 at 2:15 PM Barnabás Pőcze <[email protected]> wrote:
>
> So, sorry to be this obtuse, but I gather the proposed interface
> is a no-go in all ways, shapes, and forms in your eyes?
Yeah, not with the limitation that it only works on global list heads.
We already have a lot of different list traversal things (rcu, safe,
hlist, continue-in-the-middle etc etc), and we clearly are going to
add a new set - but I don't want to add one that is so limited that it
only works for the simplest kind of list.
Linus
On Fri, Mar 11, 2022 at 3:45 PM Linus Torvalds
<[email protected]> wrote:
>
> Yeah, not with the limitation that it only works on global list heads. [..]
"global" is the wrong word. Obviously it works on static list heads in
file - or even function - scope etc too. But you get what I'm saying..
Linus