Issue 32441: os.dup2 should return the new fd

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.

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

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)

I would just make a declaration a definition with a dummy value (0?) rather than complicating the branches.

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.

I suspect the dup3_works variable is supposed to be static.

Good catch, the code makes much more sense now :-)

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;
         ^~~

https://github.com/python/cpython/pull/5346 (merged) should fix that warning.

nosy: + vstinner
messages: + msg310752

resolution: fixed -> (no value)

messages: + msg309549

stage: patch review -> resolved