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

[John Hubbard <jhubbard@nvidia.com>]

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! :)

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