Skip to content

bpo-20104: Add flag capabilities to posix_spawn #6693

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
pablogsal merged 11 commits into from
Sep 7, 2018
Merged

bpo-20104: Add flag capabilities to posix_spawn #6693

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4982abd
Add attr capabilities to posix_spawn
pablogsal May 2, 2018
ad862cc
Add documentation for the new parameters of posix_spawn
pablogsal May 7, 2018
552b1dc
Refactor and clean tests for posix_spawn flags
pablogsal May 8, 2018
713d803
Refactor and clean implementation of parse_posix_spawn_flags
pablogsal May 8, 2018
ecd64e4
Fix scheduler test for posix_spawn flags to use correctly the policy …
pablogsal May 8, 2018
88a831c
Indent string code blocks
pablogsal May 10, 2018
0fbae92
Fix typo in the documentation
pablogsal May 14, 2018
06262a5
Regenerated the clinic files
pablogsal Jul 20, 2018
c784e0f
Make tests more strict
pablogsal Aug 31, 2018
514f55e
Use signal.NSIG for testing invalid signals
pablogsal Sep 3, 2018
abfcd6a
Accept 1 and 0 as valid values for resetids
pablogsal Sep 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion Doc/library/os.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3392,7 +3392,9 @@ written in Python, such as a mail server's external command delivery program.
subprocesses.


.. function:: posix_spawn(path, argv, env, file_actions=None)
.. function:: posix_spawn(path, argv, env, file_actions=None, /, *, \

@ZackerySpytz ZackerySpytz Jul 24, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't believe this / should be here.

@serhiy-storchaka serhiy-storchaka Jul 24, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See #6725.

setpgroup=None, resetids=False, setsigmask=(), \
setsigdef=(), scheduler=None)

Wraps the :c:func:`posix_spawn` C library API for use from Python.

Expand Down Expand Up @@ -3430,6 +3432,36 @@ written in Python, such as a mail server's external command delivery program.
:c:func:`posix_spawn_file_actions_adddup2` API calls used to prepare
for the :c:func:`posix_spawn` call itself.

The *setpgroup* argument will set the process group of the child to the value
specified. If the value specified is 0, the child's process group ID will be
made the same as its process ID. If the value of *setpgroup* is not set, the
child will inherit the parent's process group ID. This argument corresponds
to the C library :c:data:`POSIX_SPAWN_SETPGROUP` flag.

If the *resetids* argument is ``True`` it will reset the effective UID and
GID of the child to the real UID and GID of the parent process. If the
argument is ``False``, then the child retains the effective UID and GID of
the parent. In either case, if the set-user-ID and set-group-ID permission
bits are enabled on the executable file, their effect will override the
setting of the effective UID and GID. This argument corresponds to the C
library :c:data:`POSIX_SPAWN_RESETIDS` flag.

The *setsigmask* argument will set the signal mask to the signal set
specified. If the parameter is not used, then the child inherits the
parent's signal mask. This argument corresponds to the C library
:c:data:`POSIX_SPAWN_SETSIGMASK` flag.

The *sigdef* argument will reset the disposition of all signals in the set
specified. This argument corresponds to the C library
:c:data:`POSIX_SPAWN_SETSIGDEF` flag.

The *scheduler* argument must be a tuple containing the (optional) scheduler
policy and an instance of :class:`sched_param` with the scheduler parameters.
A value of ``None`` in the place of the scheduler policy indicates that is
not being provided. This argument is a combination of the C library
:c:data:`POSIX_SPAWN_SETSCHEDPARAM` and :c:data:`POSIX_SPAWN_SETSCHEDULER`
flags.

.. versionadded:: 3.7


Expand Down
145 changes: 145 additions & 0 deletions Lib/test/test_posix.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import errno
import sys
import signal
import time
import os
import platform
Expand All @@ -16,6 +17,7 @@
import tempfile
import unittest
import warnings
import textwrap

_DUMMY_SYMLINK = os.path.join(tempfile.gettempdir(),
support.TESTFN + '-dummy-symlink')
Expand Down Expand Up @@ -1476,6 +1478,149 @@ def test_empty_file_actions(self):
)
self.assertEqual(os.waitpid(pid, 0), (pid, 0))

def test_resetids_explicit_default(self):
pid = posix.posix_spawn(
sys.executable,
[sys.executable, '-c', 'pass'],
os.environ,
resetids=False
)
self.assertEqual(os.waitpid(pid, 0), (pid, 0))

@serhiy-storchaka serhiy-storchaka May 6, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Too much empty lines. Use two empty lines for separating classes. And the empty line before assertEqual() above is suspicious, the closing parenthesis is a good vertical delimiter.

def test_resetids(self):
pid = posix.posix_spawn(
sys.executable,
[sys.executable, '-c', 'pass'],
os.environ,
resetids=True
)
self.assertEqual(os.waitpid(pid, 0), (pid, 0))

def test_resetids_wrong_type(self):

@serhiy-storchaka serhiy-storchaka May 6, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we don't need so much similar tests. Needed only one test per error case. For example if the argument must be an iterable of integers in specified range, then test with a non-iterable, a list containing a non-integer, and a list containing an invalid integer. Your multiple tests test only the single kind of errors.

with self.assertRaises(TypeError):
posix.posix_spawn(sys.executable,
[sys.executable, "-c", "pass"],
os.environ, resetids=0)

@vstinner vstinner Jul 24, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the rationale for rejecting 0 as false? resetids=0 seems legit to me.

Why not using "bool" type for this parameter? Something like:

@@ -5416,7 +5411,7 @@ os.posix_spawn
     *
     setpgroup: object = NULL
         The pgroup to use with the POSIX_SPAWN_SETPGROUP flag.
-    resetids: object = False
+    resetids: bool = False
         If the value is `True` the POSIX_SPAWN_RESETIDS will be activated.
     setsigmask: object(c_default='NULL') = ()
         The sigmask to use with the POSIX_SPAWN_SETSIGMASK flag.

@pablogsal pablogsal Aug 31, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If we use bool then the truthiness value of the object will be used. This will allow the user to pass anything there (ant the boolean value of the argument will be used), right?

I would prefer to pass only True or False (maybe 0 and 1 as well) but not any object.

with self.assertRaises(TypeError):
posix.posix_spawn(sys.executable,
[sys.executable, "-c", "pass"],
os.environ, resetids=None)

def test_setpgroup(self):
pid = posix.posix_spawn(
sys.executable,
[sys.executable, '-c', 'pass'],
os.environ,
setpgroup=os.getpgrp()
)
self.assertEqual(os.waitpid(pid, 0), (pid, 0))

def test_setpgroup_wrong_type(self):
with self.assertRaises(TypeError):

@serhiy-storchaka serhiy-storchaka May 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reorder test in the order of performing corresponding checks in the code: first a non-iterable, then an iterable of non-integers, then out of range integers.

posix.posix_spawn(sys.executable,
[sys.executable, "-c", "pass"],
os.environ, setpgroup="023")

@unittest.skipUnless(hasattr(signal, 'pthread_sigmask'),
'need signal.pthread_sigmask()')
def test_setsigmask(self):
code = textwrap.dedent("""\
import _testcapi, signal
_testcapi.raise_signal(signal.SIGUSR1)""")

pid = posix.posix_spawn(
sys.executable,
[sys.executable, '-c', code],
os.environ,
setsigmask=[signal.SIGUSR1]
)
self.assertEqual(os.waitpid(pid, 0), (pid, 0))

def test_setsigmask_wrong_type(self):
with self.assertRaises(TypeError):
posix.posix_spawn(sys.executable,
[sys.executable, "-c", "pass"],
os.environ, setsigmask=34)
with self.assertRaises(TypeError):
posix.posix_spawn(sys.executable,
[sys.executable, "-c", "pass"],
os.environ, setsigmask=["j"])
with self.assertRaises(ValueError):
posix.posix_spawn(sys.executable,
[sys.executable, "-c", "pass"],
os.environ, setsigmask=[9998, 9999])

@serhiy-storchaka serhiy-storchaka May 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

setsigmask or setsigdef?

@vstinner vstinner Jul 24, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would prefer a more explicit signal.NSIG rather than arbitrary integer 9999. Maybe [signal.NSIG, signal.NSIG+1]?


@unittest.skipUnless(hasattr(signal, 'pthread_sigmask'),
'need signal.pthread_sigmask()')
def test_setsigdef(self):

@serhiy-storchaka serhiy-storchaka May 8, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be better if move this test closer to the setsigmask tests, above the setscheduler tests.

original_handler = signal.signal(signal.SIGUSR1, signal.SIG_IGN)
code = textwrap.dedent("""\
import _testcapi, signal
_testcapi.raise_signal(signal.SIGUSR1)""")
try:
pid = posix.posix_spawn(
sys.executable,
[sys.executable, '-c', code],
os.environ,
setsigdef=[signal.SIGUSR1]
)
finally:
signal.signal(signal.SIGUSR1, original_handler)

pid2, status = os.waitpid(pid, 0)
self.assertEqual(pid2, pid)
self.assertNotEqual(status, 0)

@vstinner vstinner Jul 24, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would suggest a stricter test:

    self.assertTrue(os.WIFSIGNALED(status), status)
    self.assertEqual(os.WTERMSIG(status), signal.SIGUSR1)

@vstinner vstinner Sep 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

done


def test_setsigdef_wrong_type(self):
with self.assertRaises(TypeError):
posix.posix_spawn(sys.executable,
[sys.executable, "-c", "pass"],
os.environ, setsigdef=34)
with self.assertRaises(TypeError):
posix.posix_spawn(sys.executable,
[sys.executable, "-c", "pass"],
os.environ, setsigdef=["j"])
with self.assertRaises(ValueError):
posix.posix_spawn(sys.executable,
[sys.executable, "-c", "pass"],
os.environ, setsigdef=[9998, 9999])

@unittest.skipUnless(hasattr(posix, 'sched_setscheduler'), "can't change scheduler")
def test_setscheduler_only_param(self):
policy = os.sched_getscheduler(0)
priority = os.sched_get_priority_min(policy)
code = textwrap.dedent(f"""\
import os
if os.sched_getscheduler(0) != {policy}:
os.exit(101)
if os.sched_getparam(0).sched_priority != {priority}:
os.exit(102)""")
pid = posix.posix_spawn(
sys.executable,
[sys.executable, '-c', code],
os.environ,
scheduler=(None, os.sched_param(priority))
)
self.assertEqual(os.waitpid(pid, 0), (pid, 0))

@unittest.skipUnless(hasattr(posix, 'sched_setscheduler'), "can't change scheduler")
def test_setscheduler_with_policy(self):
policy = os.sched_getscheduler(0)
priority = os.sched_get_priority_min(policy)
code = textwrap.dedent(f"""\
import os
if os.sched_getscheduler(0) != {policy}:
os.exit(101)
if os.sched_getparam(0).sched_priority != {priority}:
os.exit(102)""")
pid = posix.posix_spawn(
sys.executable,
[sys.executable, '-c', code],
os.environ,
scheduler=(policy, os.sched_param(priority))
)
self.assertEqual(os.waitpid(pid, 0), (pid, 0))

def test_multiple_file_actions(self):
file_actions = [
(os.POSIX_SPAWN_OPEN, 3, os.path.realpath(__file__), os.O_RDONLY, 0),
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2018-05-05-23-26-58.bpo-20104.tDBciE.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Added support for the `setpgroup`, `resetids`, `setsigmask`, `setsigdef` and
`scheduler` parameters of `posix_spawn`. Patch by Pablo Galindo.
40 changes: 31 additions & 9 deletions Modules/clinic/posixmodule.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading