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

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

PySequence_Check

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?

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

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.

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.

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

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.

I have made the requested changes; please review again

Thanks for making the requested changes!

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

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

News needs update to no longer mention FileActions class

@pppery Thanks! Corrected in b9db2c8

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

mention the name of the ifdef this pairs with.

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

Done in cb440d5

I have made the requested changes; please review again

Thanks for making the requested changes!

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

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

Bad indentation.

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

Too much empty lines.

PySequence_Size() can be called before PySequence_Check().

Use PySequence_Fast_GET_SIZE() instead of PySequence_Check().

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.

Use idiomatic code:

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

Return before _Py_END_SUPPRESS_IPH? This looks like a bug.

The result of the call is not checked.

Isn't it leaked in error case?

Use filesystem encoding. PyUnicode_FSDecoder() or like.

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

Should be int.

Check integer overflow when cast to mode_t.

Should be int.

Wouldn't ValueError more appropriate exception type?

Should be checked for error.

Also there are trailing spaces added by this PR.

Shouldn't errno be included in the exception?

file_actions is never NULL.

@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

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

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

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

+                  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

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

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

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

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

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

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



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


		path_error(path);

		free_string_array(envlist, envc);

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

Repository navigation

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

bpo-20104: Expose posix_spawn in the os module#5109gpshead merged 8 commits intopython:masterpython/cpython:masterfrom pablogsal:bpo20104pablogsal/cpython:bpo20104Copy head branch name to clipboard

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!

bpo-20104: Expose posix_spawn in the os module#5109
gpshead merged 8 commits into
python:masterfrom
pablogsal:bpo20104

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

pppery Jan 18, 2018 •
edited
Loading

pablogsal commented Apr 1, 2018 •
edited
Loading