On Wed, Sep 25, 2024 at 06:04:59PM GMT, Lorenzo Stoakes wrote:

On Wed, Sep 25, 2024 at 09:19:17AM GMT, Shakeel Butt wrote:

quoted

I have no idea what makes you think I am blocking the feature that you
repond in a weird tone but let me be upfront what I am asking: Let's
collectively decide which is the better option (in terms of
maintainability and extensibility) and move forward.

I'm not sure what you mean by 'weird tone'... perhaps a miscommunication?

To summarise in my view - a suggestion was made to, rather than provide the
proposed flag - a pidfd sentinel should be introduced.

Simply introducing a sentinel that represents 'the current process' without
changing interfaces that accept a pidfd would be broken - so implementing
this implies that _all_ pidfd interfaces are updated, as well as tests.

While I suggested PIDFD_SELF, I never meant that we should change every interface,
but rather adopt a sound, consistent strategy for pidfd interfaces and stick with
it for the foreseeable future.

In this case, we'd adapt process_madvise, then possibly later pidfd_send_signal, etc.
There are plenty of pidfd interfaces that don't make sense with a PIDFD_SELF. Various
other interfaces will probably never want to adopt it at all (select _can't_, other
fs syscalls such as read/write/poll/whatever would require awful handholding from
various kernel subsystems, while in that sense we would definitely require a proper
struct file/inode/whatever for each pseudo-fd, which is not exactly what we want).

I suggest doing so is, of course, entirely out of the scope of this
change. Therefore if we were to require that here - it would block the
feature while I go work on that.

I think this is pretty clear right? And I also suggest that doing so is
likely to take quite some time, and may not even have a positive outcome.

So it's not a case of 'shall we take approach A or approach B?' but rather
'should we take approach A or entirely implement a new feature B, then once
that is done, use it'.

So as to your 'collectively decide what is the better option' - in my
previous response I argued that the best approach between 'use an
unimplemented suggested entirely new feature of pidfd' vs. 'implement a
flag that would in no way block the prior approach' - a flag works better.

I just don't think it's a new feature, just an established, future-proof way
of doing things :) Your patch should remain mostly similar apart from switching
the flag check into an fd check.

quoted

By big undertaking, do you mean other syscalls that take pidfd
(pidfd_getfd, pidfd_send_signal & process_mrelease) to handle PIDFD_SELF
or something else?

I mean if you add a pidfd sentinel that represents 'the current process' it
may get passed to any interface that accepts a pidfd, so all of them have
to handle it _somehow_.

Also you'll want to update tests accordingly and clearly need to get
community buy-in for that feature.

You may want to just add a bunch of:

if (pidfd == SENTINEL)
	return -EINVAL;

It should already be there in the form of an -EBADF.

So it's not impossible my instincts are off and we can get away with simply
doing that.

On the other hand, would that be confusing? Wouldn't we need to update
documentation, manpages, etc. to say explicitly 'hey this sentinel is just
not supported'?

This is a fair point, but we could also just... not :) which I don't feel is too
wrong, since the fd works kind of like a flag here.

-- 
Pedro