bpo-20104: Expose `posix_spawn` in the os module by pablogsal · Pull Request #5109 · python/cpython

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

PySequence_Check

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

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

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

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

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

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

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

mention the name of the ifdef this pairs with.

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

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

+   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

+    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

+    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

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


Too much empty lines.

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

+        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

+                        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

+                    }
+
+                    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

+
+    _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

+}
+
+    _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

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


Isn't it leaked in error case?

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

+                    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

+                        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

+                    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

+                    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

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


Should be int.

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

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


Should be int.

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

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


Wouldn't ValueError more appropriate exception type?

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

+
+
+            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

+    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

+
+    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



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

Uh oh!

Conversation

pablogsal commented Jan 6, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

bedevere-bot commented Jan 8, 2018

Uh oh!

gpshead commented Jan 8, 2018

Uh oh!

pablogsal commented Jan 8, 2018

Uh oh!

gpshead commented Jan 8, 2018

Uh oh!

pablogsal commented Jan 10, 2018

Uh oh!

bedevere-bot commented Jan 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pppery Jan 18, 2018 • 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!

pablogsal commented Jan 18, 2018

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!

pablogsal commented Jan 28, 2018

Uh oh!

bedevere-bot commented Jan 28, 2018

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!

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!

Choose a reason for hiding this comment

pablogsal commented Jan 6, 2018 •

edited by bedevere-bot

Loading

pppery Jan 18, 2018 •

edited

Loading

pablogsal commented Apr 1, 2018 •

edited

Loading