Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os.dup2 should return the new fd #76622

Closed
benjaminp opened this issue Dec 28, 2017 · 9 comments
Closed

os.dup2 should return the new fd #76622

benjaminp opened this issue Dec 28, 2017 · 9 comments
Labels
3.7 easy extension-modules

Comments

@benjaminp
Copy link
Contributor

@benjaminp benjaminp commented Dec 28, 2017

BPO 32441
Nosy @gpshead, @vstinner, @benjaminp, @xdegaye
PRs
  • #5041
  • Files
  • without-dup3_works.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2018-01-30.07:16:36.816>
    created_at = <Date 2017-12-28.17:44:43.535>
    labels = ['extension-modules', 'easy', '3.7']
    title = 'os.dup2 should return the new fd'
    updated_at = <Date 2018-01-30.07:16:36.815>
    user = 'https://github.com/benjaminp'

    bugs.python.org fields:

    activity = <Date 2018-01-30.07:16:36.815>
    actor = 'benjamin.peterson'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-01-30.07:16:36.816>
    closer = 'benjamin.peterson'
    components = ['Extension Modules']
    creation = <Date 2017-12-28.17:44:43.535>
    creator = 'benjamin.peterson'
    dependencies = []
    files = ['47366']
    hgrepos = []
    issue_num = 32441
    keywords = ['patch', 'easy']
    message_count = 9.0
    messages = ['309135', '309197', '309503', '309538', '309549', '309578', '309585', '310752', '311239']
    nosy_count = 4.0
    nosy_names = ['gregory.p.smith', 'vstinner', 'benjamin.peterson', 'xdegaye']
    pr_nums = ['5041']
    priority = 'low'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue32441'
    versions = ['Python 3.7']

    @benjaminp
    Copy link
    Contributor Author

    @benjaminp benjaminp commented Dec 28, 2017

    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.

    @benjaminp benjaminp added 3.7 extension-modules easy labels Dec 28, 2017
    @benjaminp
    Copy link
    Contributor Author

    @benjaminp benjaminp commented Dec 29, 2017

    New changeset bbdb17d by Benjamin Peterson in branch 'master':
    return the new file descriptor from os.dup2 (closes bpo-32441) (bpo-5041)
    bbdb17d

    @xdegaye
    Copy link
    Mannequin

    @xdegaye xdegaye mannequin commented Jan 5, 2018

    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)

    @benjaminp
    Copy link
    Contributor Author

    @benjaminp benjaminp commented Jan 6, 2018

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

    @xdegaye
    Copy link
    Mannequin

    @xdegaye xdegaye mannequin commented Jan 6, 2018

    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.

    @benjaminp
    Copy link
    Contributor Author

    @benjaminp benjaminp commented Jan 6, 2018

    I suspect the dup3_works variable is supposed to be static.

    @xdegaye
    Copy link
    Mannequin

    @xdegaye xdegaye mannequin commented Jan 6, 2018

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

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jan 26, 2018

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

    @vstinner vstinner reopened this Jan 26, 2018
    @gpshead
    Copy link
    Member

    @gpshead gpshead commented Jan 30, 2018

    #5346 (merged) should fix that warning.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 easy extension-modules
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants