On 10/25/24 12:49 PM, Lorenzo Stoakes wrote:
On Fri, Oct 25, 2024 at 11:44:34AM -0700, John Hubbard wrote:
On 10/25/24 11:38 AM, Pedro Falcato wrote:
On Fri, Oct 25, 2024 at 6:41 PM John Hubbard <jhubbard@xxxxxxxxxx> wrote:
...
That seems to only apply to the kernel internally, uapi headers are
Yes.
included from userspace too (-std=c89 -pedantic doesn't know what a
gnu extension is). And uapi headers _generally_ keep to defining
constants and structs, nothing more.
OK
Because a lot of people using -ANSI- C89 are importing a very new linux
feature header.
I'll admit to being easily cowed by "you're breaking userspace" arguments.
Even when they start to get rather absurd. Because I can't easily tell where
the line is.
Maybe "-std=c89 -pedantic" is on the other side of the line. I'd like it
to be! :)
And let's ignore the hundreds of existing uses... OK.
The rules, unstated anywhere, are that we must support 1972-era C in an
optional header for a feature available only in new kernels because
somebody somewhere is using a VAX-11 and gosh darn it they can't change
their toolchain!
And you had better make sure you don't wear out those tape drums...
I don't know what the guidelines for uapi headers are nowadays, but we
generally want to not break userspace.
I think it's quite clear at this point, that we should not hold up new
work, based on concerns about handling the inline keyword, nor about
C89.
Right, but the correct solution is probably to move
pidfd_is_self_sentinel to some other place, since it's not even
supposed to be used by userspace (it's semantically useless to
userspace, and it's only two users are in the kernel, kernel/pid.c and
exit.c).
Yes, if userspace absolutely doesn't need nor want this, then putting
it in a non-uapi header does sound like the right move.
The bike shed should be blue! Wait no no, it should be red... Hang on
yellow yes! Yellow's great!
Putting a header in the right location, so as to avoid breakage here or
there, is not bikeshedding. Sorry.
No wait - did we _test_ yellow in the way I wanted...
I mean for me this isn't a big deal - we declare the defines here, it makes
sense to have a very very simple inline function.
It's not like userspace is overly hurt by this...
Also I did explain there's no obvious header to put this in in the kernel
and I'm not introducing one sorry.
ANyway if you guys feel strong enough about this, I'll respin again and
just open-code this trivial check where it's used.
No strong feelings, just hoping to help make a choice that gets you
closer to getting your patches committed.
thanks,
--
John Hubbard
