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