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.

Sign up for GitHub

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

Jump to bottom

set_inheritable(): ioctl(FIOCLEX) is available but fails with ENOTTY on Illumos #66454

Closed
igorpashev mannequin opened this issue Aug 23, 2014 · 12 comments
Closed

set_inheritable(): ioctl(FIOCLEX) is available but fails with ENOTTY on Illumos #66454

igorpashev mannequin opened this issue Aug 23, 2014 · 12 comments
Labels
build The build process and cross-build topic-IO type-bug An unexpected behavior, bug, or error

Comments

@igorpashev
Copy link
Mannequin

igorpashev mannequin commented Aug 23, 2014

BPO 22258
Nosy @vstinner, @serhiy-storchaka
Files
  • dyson-set_inheritable.patch: Patch making use of FD_CLOEXEC in set_inheritable()
  • ioctl_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 2014-09-02.09:50:08.283>
created_at = <Date 2014-08-23.13:20:24.111>
labels = ['type-bug', 'expert-IO', 'build']
title = 'set_inheritable(): ioctl(FIOCLEX) is available but fails with ENOTTY on Illumos'
updated_at = <Date 2014-09-04.07:30:27.644>
user = 'https://bugs.python.org/igorpashev'

    bugs.python.org fields:

    activity = <Date 2014-09-04.07:30:27.644>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2014-09-02.09:50:08.283>
closer = 'python-dev'
components = ['Build', 'IO']
creation = <Date 2014-08-23.13:20:24.111>
creator = 'igor.pashev'
dependencies = []
files = ['36443', '36460']
hgrepos = []
issue_num = 22258
keywords = ['patch']
message_count = 12.0
messages = ['225745', '225765', '225799', '225813', '225855', '226265', '226267', '226268', '226269', '226337', '226339', '226340']
nosy_count = 4.0
nosy_names = ['vstinner', 'python-dev', 'serhiy.storchaka', 'igor.pashev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue22258'
versions = ['Python 3.4', 'Python 3.5']
    @igorpashev
    Copy link
    Mannequin Author

    igorpashev mannequin commented Aug 23, 2014

    I've found on illumos-based OS that under some conditions python is unable to open any files because set_inheritable() fails. This happens even when building python when it cannot find (open) _sysconfigdata.py.

    The problem is that set_inheritable() first checks for FIOCLEX/FIONCLEX ioctls and tries to use them if such macros are defined. These macros can be defined at illumos, but kernel does not support them (inappropriate ioctl). Since other pieces of python already use FD_CLOEXEC unconditionally, including get_inheritable() I patched set_inheritable() to use FD_CLOEXEC. See attached patch.

    @igorpashev igorpashev mannequin added build The build process and cross-build topic-IO type-bug An unexpected behavior, bug, or error labels Aug 23, 2014
    @vstinner
    Copy link
    Member

    The patch doesn't look correct. Ioctl() reduces the overhead in term of
    syscalls (1 vs 2).

    What is the errno value on failure? We should remember that ioctl() doesn't
    work and fallback to fcntl(). It would be more portable. Similar check is
    already done for O_TMPFILE and O_CLOEXEC for example.

    See the PEP-446.

    @igorpashev
    Copy link
    Mannequin Author

    igorpashev mannequin commented Aug 24, 2014

    errno is 25 (#define ENOTTY 25 /* Inappropriate ioctl for device */)

    It does not make sense to me to call unworkable ioctl() each time before other methods :-)

    I would consider adding a configure check for working ioctl() (but it won't work for cross compilation).

    @vstinner
    Copy link
    Member

    I propose to only call ioctl() once, and then remember (in a static
    variable) that it doesn't work and then always call fcntl().

    I can work on a patch.

    @vstinner vstinner changed the title Use FD_CLOEXEC in Python/fileutils.c set_inheritable(): ioctl(FIOCLEX) is available but fails with ENOTTY on Illumos Aug 25, 2014
    @vstinner
    Copy link
    Member

    @igor: Here is a patch. Can you please apply it and run "./python -m test -v test_os" on your OS? I tried it manually on Linux by forcing the errno to ENOTTY.

    @vstinner
    Copy link
    Member

    vstinner commented Sep 2, 2014

    Ping Igor.

    @igorpashev
    Copy link
    Mannequin Author

    igorpashev mannequin commented Sep 2, 2014

    Yes, Victor. Your patch works. All os tests are passed :-)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 2, 2014

    New changeset 27cef7476f2b by Victor Stinner in branch '3.4':
    Closes bpo-22258: Fix the the internal function set_inheritable() on Illumos.
    http://hg.python.org/cpython/rev/27cef7476f2b

    New changeset 4a51c45f405b by Victor Stinner in branch 'default':
    (Merge 3.4) Closes bpo-22258: Fix the the internal function set_inheritable() on
    http://hg.python.org/cpython/rev/4a51c45f405b

    @python-dev python-dev mannequin closed this as completed Sep 2, 2014
    @vstinner
    Copy link
    Member

    vstinner commented Sep 2, 2014

    Yes, Victor. Your patch works. All os tests are passed :-)

    Thanks for the tests. I pushed my fix to Python 3.4 & 3.5.

    @serhiy-storchaka
    Copy link
    Member

    +- Issue bpo-22258: Fix the the internal function set_inheritable() on Illumos.

    "the the"

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 4, 2014

    New changeset 9d5386a22e68 by Victor Stinner in branch 'default':
    Issue bpo-22258: Fix typo in Misc/NEWS
    http://hg.python.org/cpython/rev/9d5386a22e68

    @vstinner
    Copy link
    Member

    vstinner commented Sep 4, 2014

    "the the"

    Oh, thanks for the report. It's now fixed. It would be better to have the Misc/NEWS entry in the patch directly :-(

    @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
    Assignees
    No one assigned
    Labels
    build The build process and cross-build topic-IO type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @vstinner @serhiy-storchaka