bpo-38692: Add os.pidfd_open. by benjaminp · Pull Request #17063 · python/cpython (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation20 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

benjaminp

@1st1

Thanks for working on this, Benjamin.

1st1

1st1 approved these changes Nov 5, 2019

vstinner

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for many for these low-level system-specific tools, we just refer to the man pages.
Return a file descriptor.
Availability: Linux

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

vstinner

vstinner

@njsmith

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

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

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

Sweet, sounds like we're all set then.

aeros

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a note about flags to the docs.

@benjaminp

@vstinner

@hroncok

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

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request

Dec 5, 2019

@benjaminp

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request

Jan 31, 2020

@benjaminp @shihai1991