bpo-20104: Expose `posix_spawn` in the os module #5109

pablogsal · 2018-01-06T01:17:39Z

This PR exposes posix_spawn in the os module. This also adds a new class in said module to work with posix_spawn file_actions argument.

https://bugs.python.org/issue20104

gpshead · 2018-01-07T23:57:13Z

Modules/posixmodule.c

+       argv is a list or tuple of strings and env is a dictionary
+       like posix.environ. */
+
+    if (!PyList_Check(argv) && !PyTuple_Check(argv)) {


PySequence_Check

gpshead · 2018-01-08T00:16:19Z

Lib/os.py

@@ -1076,3 +1076,46 @@ def __fspath__(self):
    @classmethod
    def __subclasshook__(cls, subclass):
        return hasattr(subclass, '__fspath__')
+
+class FileActions(object):


This class does not map well to the POSIX posix_spawn() API. It take an arbitrary ordered sequence of file_actions populated by the underlying posix_spawn_file_actions_add{dup2,open.close} C APIs.

What you really want is for the posix_spawn Python API to take a list of actions, each of which is an instance of a fileaction describing a dup2/open/close operation.

Thanks @gpshead ! That makes a lot of sense. Should I create three different classes for dup2/open/close and pass a list of these of there is a better approach?

gpshead · 2018-01-08T00:16:50Z

Lib/test/test_posix.py

+        file_actions.add_open(3,os.path.realpath(__file__),0,0)
+        file_actions.add_close(2)
+        file_actions.add_dup2(1,4)
+        pid = posix.posixspawn(sys.executable, [sys.executable, "-c", "pass"], os.environ, file_actions)


The Python API name should be .posix_spawn to match the posix C API name.

bedevere-bot · 2018-01-08T00:17:35Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

gpshead · 2018-01-08T23:03:15Z

I suggest creating namedtuple instances for each of dup2, open, and close in the module and requiring this parameter to be a sequence of those.

pablogsal · 2018-01-08T23:37:55Z

Is then ok to bring collections to the os module? I am asking this because is a big dependency.

gpshead · 2018-01-08T23:44:20Z

good point. given that, just define a trio of simple data classes of your own or some constants to indicate open/close/dup2 for use as the first element of a tuple? the "good" thing about os/posix module APIs is that they can be quite low level looking. This is the module providing a relatively raw wrapper of system calls. higher level abstrations to make using them easier often live in other modules.

pablogsal · 2018-01-10T22:39:30Z

I have made the requested changes; please review again

bedevere-bot · 2018-01-10T22:39:32Z

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.

pppery · 2018-01-18T22:13:42Z

Lib/test/test_posix.py

+        self.assertEqual(os.waitpid(pid,0),(pid,0))
+
+
+    @unittest.skipUnless(hasattr(os, 'posixspawn'), "test needs os.posix_spawn")


Shouldn't this be hasattr(os, "posix_spawn"). Same for the other test

pppery · 2018-01-18T22:13:51Z

Misc/NEWS.d/next/Core and Builtins/2018-01-06-01-14-53.bpo-20104.9DkKb8.rst

@@ -0,0 +1,2 @@
+Expose posix_spawn and a FileActions object to work with said function in


News needs update to no longer mention FileActions class

pablogsal · 2018-01-18T23:33:16Z

@pppery Thanks! Corrected in b9db2c8

gpshead · 2018-01-28T20:29:37Z

Modules/posixmodule.c

+            switch(mode) {
+
+                case POSIX_SPAWN_OPEN:
+                    if(PySequence_Size(file_actions_obj) != 5){


you need to check for errors in all of these cases: The PyXXX_AsYYY APIs can cause TypeError or OverflowError. PyLong_As APIs return -1 on error, use PyErr_Occurred() to disambiguate. Check for NULL from PyUnicode_As.

Done in cb440d5

gpshead · 2018-01-28T20:30:55Z

Modules/posixmodule.c

+
+
+}
+#endif


mention the name of the ifdef this pairs with.

gpshead · 2018-01-28T20:31:15Z

Modules/posixmodule.c

+    return PyLong_FromPid(pid);
+_Py_END_SUPPRESS_IPH
+
+path_error(path);


lack of indentation down here looks odd for everything but the "fail:" label.

Done in cb440d5

pablogsal · 2018-01-28T20:45:13Z

I have made the requested changes; please review again

bedevere-bot · 2018-01-28T20:45:15Z

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.

serhiy-storchaka · 2018-03-31T08:36:48Z

Modules/posixmodule.c

+   argv is a list or tuple of strings and env is a dictionary
+       like posix.environ. */
+
+    if (!PySequence_Check(argv)){


Why not (!PyList_Check(argv) && !PyTuple_Check(argv)) as in other places?

serhiy-storchaka · 2018-03-31T08:37:08Z

Modules/posixmodule.c

+    Py_ssize_t argc, envc;
+
+    /* posix_spawn has three arguments: (path, argv, env), where
+   argv is a list or tuple of strings and env is a dictionary


Bad indentation.

serhiy-storchaka · 2018-03-31T08:39:23Z

Modules/posixmodule.c

+    posix_spawn_file_actions_t *file_actionsp = NULL;
+    if (file_actions  != NULL && file_actions != Py_None){
+        posix_spawn_file_actions_t _file_actions;
+        if(posix_spawn_file_actions_init(&_file_actions) != 0){


Doesn't conform PEP 7. Needed spaces after "if" and before "{".

serhiy-storchaka · 2018-03-31T08:39:54Z

Modules/posixmodule.c

+                            "Error initializing file actions");
+            goto fail;
+        }
+


Too much empty lines.

serhiy-storchaka · 2018-03-31T08:40:57Z

Modules/posixmodule.c

+        for (int i = 0; i < PySequence_Fast_GET_SIZE(seq); ++i) {
+            file_actions_obj = PySequence_Fast_GET_ITEM(seq, i);
+
+            if(!PySequence_Check(file_actions_obj) | !PySequence_Size(file_actions_obj)){


PySequence_Size() can be called before PySequence_Check().

Use PySequence_Fast_GET_SIZE() instead of PySequence_Check().

serhiy-storchaka · 2018-03-31T08:45:18Z

Modules/posixmodule.c

+                        goto fail;
+                    }
+
+                    long open_fd = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 1));


PyLong_AsLong() can call arbitrary Python code. This can cause changing the size of file_actions_obj. Following PySequence_GetItem() can fail.

I would require file_actions_obj to be a tuple and use PyArg_ParseTuple() for parsing it.

serhiy-storchaka · 2018-03-31T08:46:50Z

Modules/posixmodule.c

+                    }
+
+                    long open_fd = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 1));
+                    if(PyErr_Occurred()) {


Use idiomatic code:

if (open_fd == -1 && PyErr_Occurred()) {

serhiy-storchaka · 2018-03-31T08:50:05Z

Modules/posixmodule.c

+
+    _Py_BEGIN_SUPPRESS_IPH
+        posix_spawn(&pid, path->narrow, file_actionsp, NULL, argvlist, envlist);
+        return PyLong_FromPid(pid);


Return before _Py_END_SUPPRESS_IPH? This looks like a bug.

serhiy-storchaka · 2018-03-31T08:51:27Z

Modules/posixmodule.c

+}
+
+    _Py_BEGIN_SUPPRESS_IPH
+        posix_spawn(&pid, path->narrow, file_actionsp, NULL, argvlist, envlist);


The result of the call is not checked.

serhiy-storchaka · 2018-03-31T08:52:24Z

Modules/posixmodule.c

+
+    path_error(path);
+
+    free_string_array(envlist, envc);


Isn't it leaked in error case?

serhiy-storchaka · 2018-04-01T11:07:24Z

Modules/posixmodule.c

+                    if(PyErr_Occurred()) {
+                        goto fail;
+                    }
+                    const char* open_path = PyUnicode_AsUTF8(PySequence_GetItem(file_actions_obj, 2));


Use filesystem encoding. PyUnicode_FSDecoder() or like.

serhiy-storchaka · 2018-04-01T11:08:24Z

Modules/posixmodule.c

+                        goto fail;
+                    }
+
+                    long open_fd = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 1));


open_fd should be of type int. Use _PyLong_AsInt().

serhiy-storchaka · 2018-04-01T11:08:47Z

Modules/posixmodule.c

+                    if(open_path == NULL){
+                        goto fail;
+                    }
+                    long open_oflag = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 3));


Should be int.

serhiy-storchaka · 2018-04-01T11:09:34Z

Modules/posixmodule.c

+                    if(PyErr_Occurred()) {
+                        goto fail;
+                    }
+                    long open_mode = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 4));


Check integer overflow when cast to mode_t.

serhiy-storchaka · 2018-04-01T11:09:58Z

Modules/posixmodule.c

+                        goto fail;
+                    }
+
+                    long close_fd = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 1));


Should be int.

serhiy-storchaka · 2018-04-01T11:10:21Z

Modules/posixmodule.c

+                        goto fail;
+                    }
+
+                    long fd1 = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 1));


Should be int.

serhiy-storchaka · 2018-04-01T11:11:22Z

Modules/posixmodule.c

+                    break;
+
+                default:
+                    PyErr_SetString(PyExc_TypeError,"Unknown file_actions identifier");


Wouldn't ValueError more appropriate exception type?

serhiy-storchaka · 2018-04-01T11:12:21Z

Modules/posixmodule.c

+
+
+            mode_obj = PySequence_Fast_GET_ITEM(file_actions_obj, 0);
+            int mode = PyLong_AsLong(mode_obj);


Should be checked for error.

serhiy-storchaka · 2018-04-01T11:17:13Z

Also there are trailing spaces added by this PR.

serhiy-storchaka · 2018-04-01T11:22:11Z

Modules/posixmodule.c

+    if (file_actions  != NULL && file_actions != Py_None){
+        posix_spawn_file_actions_t _file_actions;
+        if(posix_spawn_file_actions_init(&_file_actions) != 0){
+            PyErr_SetString(PyExc_TypeError,


Shouldn't errno be included in the exception?

serhiy-storchaka · 2018-04-01T14:57:53Z

Modules/posixmodule.c

+
+    pid_t pid;
+    posix_spawn_file_actions_t *file_actionsp = NULL;
+    if (file_actions  != NULL && file_actions != Py_None){


file_actions is never NULL.

pablogsal · 2018-04-01T15:09:48Z

@serhiy-storchaka I am trying to correct all these new issues in Corrected in #6331. Please, notice that some of them (like the result of posix_spawn not checked and the return before _Py_END_SUPPRESS_IPH) were already corrected as this is an outdated diff. Thanks for all the effort with this!

the-knights-who-say-ni added the CLA signed label Jan 6, 2018

bedevere-bot added the awaiting review label Jan 6, 2018

pablogsal force-pushed the bpo20104 branch 6 times, most recently from 360963b to c1fce61 Compare January 6, 2018 02:26

gpshead requested changes Jan 8, 2018

View reviewed changes

bedevere-bot removed the awaiting review label Jan 8, 2018

bedevere-bot added the awaiting changes label Jan 8, 2018

bedevere-bot added awaiting change review and removed awaiting changes labels Jan 10, 2018

serhiy-storchaka changed the title ~~bpo-20104 Expose expose posix_spawn in the os module~~ bpo-20104 Expose posix_spawn in the os module Jan 12, 2018

serhiy-storchaka changed the title ~~bpo-20104 Expose posix_spawn in the os module~~ bpo-20104: Expose posix_spawn in the os module Jan 12, 2018

pppery reviewed Jan 18, 2018

View reviewed changes

pablogsal force-pushed the bpo20104 branch 7 times, most recently from 7c9d964 to b25b7d1 Compare January 21, 2018 02:12

gpshead reviewed Jan 28, 2018

View reviewed changes

pablogsal added 8 commits January 28, 2018 20:35

Expose posix_spawn in os module

f138f0a

Add News entry

bd39bce

Update argument clinic files

e8ca1d4

Adapt posix_spawn API to pythonic posix compatible version

7ea83ed

Clean implementation of posix_spawn

90ed199

Fix function name in tests and update NEWS

6ba804d

Fixed configure script

88831ec

Add error checking, correct style

cb440d5

pablogsal force-pushed the bpo20104 branch from b25b7d1 to cb440d5 Compare January 28, 2018 20:44

gpshead approved these changes Jan 29, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jan 29, 2018

gpshead merged commit 6c6ddf9 into python:master Jan 29, 2018

bedevere-bot removed the awaiting merge label Jan 29, 2018

pablogsal deleted the bpo20104 branch January 29, 2018 02:12

serhiy-storchaka reviewed Mar 31, 2018

View reviewed changes

serhiy-storchaka mentioned this pull request Mar 31, 2018

bpo-33191: Fix refleak in posix_spawn #6315

Closed

serhiy-storchaka reviewed Apr 1, 2018

View reviewed changes

pablogsal mentioned this pull request Apr 1, 2018

bpo-33191: Fix refleaks, style and other issues for os.posix_spawn #6331

Closed

		self.assertEqual(os.waitpid(pid,0),(pid,0))


		@unittest.skipUnless(hasattr(os, 'posixspawn'), "test needs os.posix_spawn")

		@@ -0,0 +1,2 @@
		Expose posix_spawn and a FileActions object to work with said function in



		mode_obj = PySequence_Fast_GET_ITEM(file_actions_obj, 0);
		int mode = PyLong_AsLong(mode_obj);



		}
		#endif

bpo-20104: Expose posix_spawn in the os module #5109

bpo-20104: Expose posix_spawn in the os module #5109

Conversation

pablogsal commented Jan 6, 2018 • edited by bedevere-bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Jan 8, 2018

gpshead commented Jan 8, 2018

pablogsal commented Jan 8, 2018

gpshead commented Jan 8, 2018

pablogsal commented Jan 10, 2018

bedevere-bot commented Jan 10, 2018

Choose a reason for hiding this comment

pppery Jan 18, 2018 • edited Loading

Choose a reason for hiding this comment

pablogsal commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablogsal commented Jan 28, 2018

bedevere-bot commented Jan 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka commented Apr 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablogsal commented Apr 1, 2018 • edited Loading

bpo-20104: Expose `posix_spawn` in the os module #5109

bpo-20104: Expose `posix_spawn` in the os module #5109

pablogsal commented Jan 6, 2018 •

edited by bedevere-bot

Loading

pppery Jan 18, 2018 •

edited

Loading

pablogsal commented Apr 1, 2018 •

edited

Loading