classification
Title: set_inheritable(): ioctl(FIOCLEX) is available but fails with ENOTTY on Illumos
Type: behavior Stage:
Components: Build, IO Versions: Python 3.5, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: haypo, igor.pashev, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2014-08-23 13:20 by igor.pashev, last changed 2014-09-04 07:30 by haypo. This issue is now closed.

Files
File name Uploaded Description Edit
dyson-set_inheritable.patch igor.pashev, 2014-08-23 13:20 Patch making use of FD_CLOEXEC in set_inheritable()
ioctl_works.patch haypo, 2014-08-25 01:00 review
Messages (12)
msg225745 - (view) Author: Igor Pashev (igor.pashev) * Date: 2014-08-23 13:20
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.
msg225765 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-08-23 21:27
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.
msg225799 - (view) Author: Igor Pashev (igor.pashev) * Date: 2014-08-24 07:39
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).
msg225813 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-08-24 12:41
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.
msg225855 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-08-25 01:00
@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.
msg226265 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-09-02 07:53
Ping Igor.
msg226267 - (view) Author: Igor Pashev (igor.pashev) * Date: 2014-09-02 08:38
Yes, Victor. Your patch works. All os tests are passed :-)
msg226268 - (view) Author: Roundup Robot (python-dev) Date: 2014-09-02 09:50
New changeset 27cef7476f2b by Victor Stinner in branch '3.4':
Closes #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 #22258: Fix the the internal function set_inheritable() on
http://hg.python.org/cpython/rev/4a51c45f405b
msg226269 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-09-02 09:50
> Yes, Victor. Your patch works. All os tests are passed :-)

Thanks for the tests. I pushed my fix to Python 3.4 & 3.5.
msg226337 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-09-04 06:27
> +- Issue #22258: Fix the the internal function set_inheritable() on Illumos.

"the the"
msg226339 - (view) Author: Roundup Robot (python-dev) Date: 2014-09-04 07:29
New changeset 9d5386a22e68 by Victor Stinner in branch 'default':
Issue #22258: Fix typo in Misc/NEWS
http://hg.python.org/cpython/rev/9d5386a22e68
msg226340 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-09-04 07:30
> "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 :-(
History
Date User Action Args
2014-09-04 07:30:27hayposetmessages: + msg226340
2014-09-04 07:29:48python-devsetmessages: + msg226339
2014-09-04 06:27:13serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg226337
2014-09-02 09:50:57hayposetstage: resolved ->
messages: + msg226269
versions: + Python 3.5
2014-09-02 09:50:08python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg226268

resolution: fixed
stage: resolved
2014-09-02 08:38:33igor.pashevsetmessages: + msg226267
2014-09-02 07:53:07hayposetmessages: + msg226265
2014-08-25 01:00:12hayposetfiles: + ioctl_works.patch

messages: + msg225855
2014-08-25 00:51:05hayposettitle: Use FD_CLOEXEC in Python/fileutils.c -> set_inheritable(): ioctl(FIOCLEX) is available but fails with ENOTTY on Illumos
2014-08-24 12:41:12hayposetmessages: + msg225813
2014-08-24 07:39:10igor.pashevsetmessages: + msg225799
2014-08-23 21:27:29hayposetmessages: + msg225765
2014-08-23 13:47:44pitrousetnosy: + haypo
2014-08-23 13:20:24igor.pashevcreate