classification
Title: os.dup2 should return the new fd
Type: Stage: resolved
Components: Extension Modules Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, gregory.p.smith, vstinner, xdegaye
Priority: low Keywords: easy, patch

Created on 2017-12-28 17:44 by benjamin.peterson, last changed 2018-01-30 07:16 by benjamin.peterson. This issue is now closed.

Files
File name Uploaded Description Edit
without-dup3_works.patch xdegaye, 2018-01-06 15:05
Pull Requests
URL Status Linked Edit
PR 5041 merged benjamin.peterson, 2017-12-29 06:57
Messages (9)
msg309135 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2017-12-28 17:44
os.dup2 currently always None. However, the underlying standard Unix function returns the new file descriptor (i.e., the second argument) on success. We should follow that convention, too.
msg309197 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2017-12-29 21:13
New changeset bbdb17d19bb1d5443ca4417254e014ad64c04540 by Benjamin Peterson in branch 'master':
return the new file descriptor from os.dup2 (closes bpo-32441) (#5041)
https://github.com/python/cpython/commit/bbdb17d19bb1d5443ca4417254e014ad64c04540
msg309503 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2018-01-05 12:27
gcc is a little bit lost and prints now the following (false) warning:

gcc -pthread -Wno-unused-result -Wsign-compare -g -Og -Wall -Wstrict-prototypes    -std=c99 -Wextra
-Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration   -I. -I./Include    -DPy_BUILD_CORE  -c ./Modules/posixmodule.c -o Modules/posixmodule.o./Modules/posixmodule.c: In function ‘os_dup2_impl’:
./Modules/posixmodule.c:7785:9: warning: ‘res’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     int res;
         ^~~


The following change fools gcc that does not print anymore the warning:

diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c
index 47b79fcc79..90d73daf97 100644
--- a/Modules/posixmodule.c
+++ b/Modules/posixmodule.c
@@ -7845,7 +7845,7 @@ os_dup2_impl(PyObject *module, int fd, int fd2, int inheritable)
         }
     }
 
-    if (inheritable || dup3_works == 0)
+    if (inheritable || (!inheritable && dup3_works == 0))
     {
 #endif
         Py_BEGIN_ALLOW_THREADS


The change does not modify the behavior:
* dup3_works == 0 is equivalent to ((inheritable && dup3_works == 0) || (!inheritable && dup3_works == 0))
* (inheritable && dup3_works == 0) is always false
* hence dup3_works == 0 is equivalent to (!inheritable && dup3_works == 0)
msg309538 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2018-01-06 07:09
I would just make a declaration a definition with a dummy value (0?) rather than complicating the branches.
msg309549 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2018-01-06 15:05
Both the change in my previous post or using a definition for 'res' are not needed if 'dup3_works' is removed. That would make the code more clear for both gcc and a human reader. The attached patch removes it.

Using a definition for 'res' LGTM also.
msg309578 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2018-01-06 20:52
I suspect the dup3_works variable is supposed to be static.
msg309585 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2018-01-06 22:20
Good catch, the code makes much more sense now :-)
msg310752 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-01-26 11:03
The change introduced a warning. Can someone please take a look (and maybe fix it)?

./Modules/posixmodule.c: In function 'os_dup2_impl':
./Modules/posixmodule.c:7785:9: warning: 'res' may be used uninitialized in this function [-Wmaybe-uninitialized]
     int res;
         ^~~
msg311239 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-01-30 06:05
https://github.com/python/cpython/pull/5346 (merged) should fix that warning.
History
Date User Action Args
2018-01-30 07:16:36benjamin.petersonsetstatus: open -> closed
resolution: fixed
2018-01-30 06:05:27gregory.p.smithsetnosy: + gregory.p.smith
messages: + msg311239
2018-01-26 11:03:13vstinnersetstatus: closed -> open

nosy: + vstinner
messages: + msg310752

resolution: fixed -> (no value)
2018-01-06 22:20:53xdegayesetmessages: + msg309585
2018-01-06 20:52:28benjamin.petersonsetmessages: + msg309578
2018-01-06 15:05:56xdegayesetfiles: + without-dup3_works.patch

messages: + msg309549
2018-01-06 07:09:27benjamin.petersonsetmessages: + msg309538
2018-01-05 12:27:44xdegayesetnosy: + xdegaye
messages: + msg309503
2017-12-29 21:13:08benjamin.petersonsetstatus: open -> closed
resolution: fixed
messages: + msg309197

stage: patch review -> resolved
2017-12-29 06:57:03benjamin.petersonsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request4924
2017-12-28 17:44:43benjamin.petersoncreate