bpo-38692: Add os.pidfd_open. #17063

benjaminp · 2019-11-05T15:22:30Z

https://bugs.python.org/issue38692

1st1 · 2019-11-05T15:26:48Z

Thanks for working on this, Benjamin.

vstinner · 2019-11-05T15:27:52Z

Lib/test/test_posix.py

+                self.skipTest("system does not support pidfd_open")
+            if e.errno != errno.EINVAL:
+                raise
+        os.close(os.pidfd_open(os.getpid(), 0))


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)

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.

vstinner · 2019-11-05T15:31:03Z

Lib/test/test_posix.py

+            if e.errno == errno.ENOSYS:
+                self.skipTest("system does not support pidfd_open")
+            if e.errno != errno.EINVAL:
+                raise


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.

Good suggestion.

vstinner · 2019-11-05T15:32:00Z

Modules/posixmodule.c

+  pid: pid_t
+  flags: unsigned_int = 0
+
+Return a file descriptor.


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.

Yes, for many for these low-level system-specific tools, we just refer to the man pages.

vstinner · 2019-11-05T15:32:41Z

Modules/posixmodule.c

+
+Return a file descriptor.
+
+Availability: Linux


I suggest to no document availability here, since this part is usually not maintained and so quickly outdated :-/

Okay, but it's certainly traditional to list it in the reST docs.

Doc/library/os.rst

njsmith · 2019-11-05T17:18:28Z

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.

benjaminp · 2019-11-05T17:39:11Z

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.

vstinner · 2019-11-05T17:46:55Z

I checked AMD64 Fedora Rawhide 3.x buildbot: it runs Linux 5.4 (release 5.4.0-0.rc5.git0.1.fc32.x86_64).

benjaminp · 2019-11-05T18:24:04Z

Sweet, sounds like we're all set then.

aeros · 2019-11-05T21:27:16Z

Modules/clinic/posixmodule.c.h

+"Return a file descriptor referring to the process *pid*.\n"
+"\n"
+"The descriptor can be used to perform process management without races and\n"
+"signals.");


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.

Suggested change

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

I added a note about flags to the docs.

vstinner · 2019-11-06T17:54:03Z

Perfect, thanks ;-)

hroncok · 2019-11-20T10:44:05Z

Just a heads up: In Fedora, I am getting:

======================================================================
FAIL: test_pidfd_open (test.test_posix.PosixTester)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builddir/build/BUILD/Python-3.9.0a1/Lib/test/test_posix.py", line 1479, in test_pidfd_open
    self.assertEqual(cm.exception.errno, errno.EINVAL)
AssertionError: 1 != 22

----------------------------------------------------------------------

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.

https://bugzilla.redhat.com/show_bug.cgi?id=1774417#c2

njsmith · 2019-11-20T10:53:23Z

Sounds like you're using some kind of buggy sandbox that's returning EPERM for unrecognized syscalls: https://bugs.python.org/issue38692#msg356235

the-knights-who-say-ni added the CLA signed label Nov 5, 2019

bedevere-bot added the awaiting core review label Nov 5, 2019

1st1 approved these changes Nov 5, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Nov 5, 2019

vstinner reviewed Nov 5, 2019

View reviewed changes

Doc/library/os.rst Show resolved Hide resolved

vstinner reviewed Nov 5, 2019

View reviewed changes

Doc/library/os.rst Show resolved Hide resolved

aeros reviewed Nov 5, 2019

View reviewed changes

bpo-38692: Add os.pidfd_open.

6e63f9e

benjaminp merged commit 6c4c45e into python:master Nov 6, 2019

benjaminp deleted the expose-pidfd_open branch November 6, 2019 03:21

bedevere-bot removed the awaiting merge label Nov 6, 2019

njsmith mentioned this pull request Nov 6, 2019

use pidfd on Linux when available python-trio/trio#1241

Closed

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019

bpo-38692: Add os.pidfd_open. (pythonGH-17063)

afacc51

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020

bpo-38692: Add os.pidfd_open. (pythonGH-17063)

6da90e3

-"signals.");
+"signals.\n"
+"\n"
+"The *flags* argument is reserved for future usage.");

Uh oh!

bpo-38692: Add os.pidfd_open. #17063

bpo-38692: Add os.pidfd_open. #17063

Uh oh!

Conversation

benjaminp commented Nov 5, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1st1 commented Nov 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

njsmith commented Nov 5, 2019

Uh oh!

benjaminp commented Nov 5, 2019

Uh oh!

vstinner commented Nov 5, 2019

Uh oh!

benjaminp commented Nov 5, 2019

Uh oh!

aeros Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Nov 6, 2019

Uh oh!

hroncok commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

njsmith commented Nov 20, 2019

Uh oh!

Uh oh!

benjaminp commented Nov 5, 2019 •

edited by bedevere-bot

Loading

aeros Nov 5, 2019 •

edited

Loading

hroncok commented Nov 20, 2019 •

edited

Loading