Re: [PATCH v5 2/5] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Fri, Oct 25, 2024 at 01:31:49PM -0700, John Hubbard wrote:
> 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@nvidia.com> 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! :)

Well, apparently not...

>
> >
> > 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.

There are 312 uses of "static inline" already in UAPI headers, not all
quite as obscure as claimed.

Specifically requiring me and only me to support ansi C89 for a theorised
scenario is in my opinion bikeshedding, but I don't want to get into an
argument about something so petty :)

>
> >
> > 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.

I mean, you are saying I am breaking things and implying the series is
blocked on this, that sounds like a strong opinion, but again I'm not going
to argue.

As with the requirement that I, only for my part of the change, must fix up
test header import, while I disagree I should be doing the fix, I did it
anyway as I am accommodating and reasonable.

So fine - I'll respin and just open-code this as it's trivial and there's
no (other) sensible place to put it anyway.

A P.S. though - a very NOT theoretical issue with userspace is the import
of linux/fcntl.h in pidfd.h which seems to me to have been imported solely
for the kernel's sake.

A gentle suggestion (it seems I can't win - gentle suggestions are ignored,
tongue-in-cheek parody is taken to be mean... but anyway) is to do
something like:

#ifdef __KERNEL__
#include <linux/fcntl.h>
#else
#include <fcntl.h>
#endif

At the top of the pidfd.h header. This must surely sting a _lot_ of people
in userland otherwise.

But this is out of scope for this change.