bpo-20104: Expose `posix_spawn` in the os module by pablogsal · Pull Request #5109 · 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

Conversation42 Commits8 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 }})

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

argv is a list or tuple of strings and env is a dictionary
like posix.environ. */

if (!PyList_Check(argv) && !PyTuple_Check(argv)) {

Choose a reason for hiding this comment

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

PySequence_Check

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

class FileActions(object):

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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?

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)

Choose a reason for hiding this comment

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

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.

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

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


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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

News needs update to no longer mention FileActions class

@pppery Thanks! Corrected in b9db2c8

switch(mode) {

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

mention the name of the ifdef this pairs with.

return PyLong_FromPid(pid);
_Py_END_SUPPRESS_IPH

path_error(path);

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

argv is a list or tuple of strings and env is a dictionary
like posix.environ. */

if (!PySequence_Check(argv)){

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Bad indentation.

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Too much empty lines.

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

Choose a reason for hiding this comment

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

PySequence_Size() can be called before PySequence_Check().

Use PySequence_Fast_GET_SIZE() instead of PySequence_Check().

goto fail;
}

long open_fd = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 1));

Choose a reason for hiding this comment

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

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.

}

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

Choose a reason for hiding this comment

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

Use idiomatic code:

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


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

Choose a reason for hiding this comment

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

Return before _Py_END_SUPPRESS_IPH? This looks like a bug.

}

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

Choose a reason for hiding this comment

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

The result of the call is not checked.


path_error(path);

free_string_array(envlist, envc);

Choose a reason for hiding this comment

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

Isn't it leaked in error case?

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

Choose a reason for hiding this comment

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

Use filesystem encoding. PyUnicode_FSDecoder() or like.

goto fail;
}

long open_fd = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 1));

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Should be int.

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

Choose a reason for hiding this comment

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

Check integer overflow when cast to mode_t.

goto fail;
}

long close_fd = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 1));

Choose a reason for hiding this comment

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

Should be int.

goto fail;
}

long fd1 = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 1));

Choose a reason for hiding this comment

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

Should be int.

break;

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

Choose a reason for hiding this comment

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

Wouldn't ValueError more appropriate exception type?



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

Choose a reason for hiding this comment

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

Should be checked for error.

Also there are trailing spaces added by this PR.

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,

Choose a reason for hiding this comment

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

Shouldn't errno be included in the exception?


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

Choose a reason for hiding this comment

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

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!

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

Conversation

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

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

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