bpo-38692: Add os.pidfd_open.#17063
bpo-38692: Add os.pidfd_open.#17063benjaminp merged 1 commit intopython:masterfrom benjaminp:expose-pidfd_open
Conversation
|
Thanks for working on this, Benjamin. |
Lib/test/test_posix.py
Outdated
There was a problem hiding this comment.
I suggest to remove ", 0" flags and add a few more tests:
pid = os.pidfd_open(os.getpid())
try:
self.assertIsInstance(pid, int)
self.assertGreaterEqual(pid, 0)
finally:
os.close(pid)
There was a problem hiding this comment.
The reason I pass 0 is to make sure the flags argument exists and "works".
I don't think your other suggested tests add much value. Calling close on such an invalid value will probably fail anyway.
Lib/test/test_posix.py
Outdated
There was a problem hiding this comment.
I suggest to first test os.pidfd_open(os.getpid()) to handle ENOSYS/skipTest, and then test pidfd_open(-1).
I would prefer to use something like:
with self.assertRaises(OSError) as cm:
os.pidfd_open(-1)
self.assertEqual(cm.exception.errno, errno.EINVAL)
Your current test does not fail if the call succeed.
Modules/posixmodule.c
Outdated
There was a problem hiding this comment.
I would prefer to repeat your documentation here:
Return a file descriptor referring to the process pid. This descriptor can
be used to perform process management without races and signals.
Note: The manual page http://man7.org/linux/man-pages/man2/pidfd_open.2.html says:
Obtain a file descriptor that refers to a process.
There was a problem hiding this comment.
Yes, for many for these low-level system-specific tools, we just refer to the man pages.
Modules/posixmodule.c
Outdated
There was a problem hiding this comment.
I suggest to no document availability here, since this part is usually not maintained and so quickly outdated :-/
There was a problem hiding this comment.
Okay, but it's certainly traditional to list it in the reST docs.
|
How are you testing this? AFAICT it's not currently possible to run a pidfd-supporting kernel in any of the hosted CI services. (Except maybe via User-Mode Linux, which I've seriously considered, but it's non-trivial.) Has anyone checked whether we have a buildbot running a new enough kernel? Having some kind of CI coverage seems important. |
|
I'm running a 5.3 kernel locally. The lack of CI is indeed troubling, but I expect it to be a temporary situation on the order of months. The next Ubunu LTS 20.04 will presumably support pidfds. I'm hoping CI will be available before 3.9 is released. |
|
I checked AMD64 Fedora Rawhide 3.x buildbot: it runs Linux 5.4 (release 5.4.0-0.rc5.git0.1.fc32.x86_64). |
|
Sweet, sounds like we're all set then. |
Modules/clinic/posixmodule.c.h
Outdated
There was a problem hiding this comment.
Is it worth briefly mentioning that the flags argument currently is just reserved for future usage in the docstring, to explain that it does not current have a functional purpose? The documentation links to the manpage (that should probably be adequate for the docs), but I think we could be more explicit in the docstring.
| "signals."); | |
| "signals.\n" | |
| "\n" | |
| "The *flags* argument is reserved for future usage."); |
Edit: There's a valid argument that this area isn't as frequently maintained, but it would have to be updated anyways if the flags arg has a functional purpose at some point. So, I think it would be worth adding, and creates very little to no additional maintenance cost.
There was a problem hiding this comment.
I added a note about flags to the docs.
|
Perfect, thanks ;-) |
|
Just a heads up: In Fedora, I am getting: When building Python 3.9.0a1. I'm still investigating as I can only get the problem in local chroot, but not in the remote build system. EDIT: The remote build system has an older kernel. |
|
Sounds like you're using some kind of buggy sandbox that's returning EPERM for unrecognized syscalls: https://bugs.python.org/issue38692#msg356235 |
https://bugs.python.org/issue38692