classification
Title: io.FileIO and io.open should support openat
Type: enhancement Stage: resolved
Components: IO, Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: rosslagerwall Nosy List: Arfrever, amaury.forgeotdarc, benjamin.peterson, eric.araujo, eryksun, haypo, mmarkk, nadeem.vawda, ned.deily, neologix, pitrou, python-dev, rosslagerwall, terry.reedy
Priority: normal Keywords: patch

Created on 2011-08-20 21:00 by pitrou, last changed 2015-12-22 21:37 by eryksun. This issue is now closed.

Files
File name Uploaded Description Edit
i12797.patch rosslagerwall, 2011-09-30 06:10
opener.patch rosslagerwall, 2011-10-30 05:32 review
opener_v2.patch rosslagerwall, 2011-10-30 19:10 review
Messages (29)
msg142560 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-20 21:00
Right now it is painful to integrate openat() with the normal IO classes. You have to figure out the low-level flags yourself (i.e. replicate the logic and error handling from the FileIO constructor), then replicate the open() logic yourself (because you want to set the name attribute on the FileIO object before wrapping it).

Therefore it would be nice if the FileIO constructor and the open() function supported openat natively. I see two possibilities:
- allow a (dirfd, name) tuple for the first "file" argument
- allow an optional dirfd argument at the end of the arglist
msg142565 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2011-08-20 21:17
A third idea is to find a way to override the low-level open() function (the one that returns a fd).

openat() seems to exist only on Linux, so I'm -1 on adding new parameters to support this function only.
msg142568 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-20 21:21
> A third idea is to find a way to override the low-level open()
> function (the one that returns a fd).

Why not. It would e.g. allow to use CreateFile under Windows (the hg
guys do this in order to change the "sharing" mode to something more
laxist).

> openat() seems to exist only on Linux, so I'm -1 on adding new
> parameters to support this function only.

openat() is POSIX:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
msg142578 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-08-20 22:22
> allow an optional dirfd argument at the end of the arglist

I prefer this suggestion.

I didn't know openat(). Antoine told me that it can be used, for example, to fix security vulnerabilities like #4489.
msg142581 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2011-08-20 23:34
I believe openat is new to POSIX (mandatory as of POSIX 2008).  For example, it's not currently in OS X and apparently was first added to FreeBSD in 8.0.  So it would have to be checked by configure and documented as platform-dependent.
msg142582 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-20 23:42
> I believe openat is new to POSIX (mandatory as of POSIX 2008).  For
> example, it's not currently in OS X and apparently was first added to
> FreeBSD in 8.0.  So it would have to be checked by configure and
> documented as platform-dependent.

We already have os.openat:
http://docs.python.org/dev/library/os.html#os.openat

This request is to make it easier to use with the high-level IO classes.
msg142588 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2011-08-21 00:01
> We already have os.openat
Ah, right.  The comment still applies, though, to future documentation of the proposed feature.  +1 on it.
msg143112 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2011-08-28 17:49
I prefer a new parameter either at the end of the arglist or possibly keyword only. The idea for both variations is to let typical users ignore the option, which would be hard to do if it is part of the prime parameter. The idea for keyword only is that we might want to add other rarely used but useful options. They have no natural order, and having say, 8 positional params is pretty wretched. (I have worked with such APIs.)
msg144674 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2011-09-30 06:10
Attached is a patch which adds dirfd= as a keyword argument.
msg145741 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-17 17:40
> Attached is a patch which adds dirfd= as a keyword argument.

Thanks. Although, on second thought, I'm not sure whether Amaury's idea (allowing a custom opener) is not better... Thoughts?
msg145849 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2011-10-18 18:16
I guess that would make it more general...

I'll play around with it for a bit. It mustn't become too hard to use though since the original point was to simplify the opening of files :-)
msg146612 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-10-29 15:57
> Thanks. Although, on second thought, I'm not sure whether Amaury's 
> idea (allowing a custom opener) is not better... Thoughts?

+1.
This would also address issues #12760 and #12105.
msg146613 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2011-10-29 16:16
What would you envisage the API for the custom opener to look like?
msg146614 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-29 16:23
> What would you envisage the API for the custom opener to look like?

The same as os.open(), I would say.
msg146616 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2011-10-29 18:33
Before I implement it properly, is this the kind of api that's desired?

"""
import os
import io

class MyOpener:
    def __init__(self, dirname):
        self.dirfd = os.open(dirname, os.O_RDONLY)

    def open(self, path, flags, mode):
        return os.openat(self.dirfd, path, flags, mode)

myop = MyOpener("/tmp")
f = open("testfile", "w", opener=myop.open)
f.write("hello")
"""
msg146617 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-29 19:21
> Before I implement it properly, is this the kind of api that's desired?

Yes, although I think most people would use a closure instead of a dedicated class.
msg146626 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2011-10-30 05:32
The attached patch adds the opener keyword + tests.
msg146647 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-30 17:30
Here is my quick review:
- shouldn't the opener also get the third open() argument (although it currently seems to always be 0o666)?
- when fdobj is NULL, you shouldn't override the original error
- PyLong_AsLong can fail (if the opener returns too large an int), you should check for that

Thank you!
msg146650 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2011-10-30 18:15
Also, the documentation should indicate what exactly is supposed to be returned by "opener".
msg146654 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2011-10-30 19:10
Updated patch:
* checks for long overflow
* raises original exception if opener returns null
* makes it explicit that "opener" must return an open file descriptor.

I don't think that mode should be passed in since it is not specified in the parameters to open() (and always defaults to 0o666 anyway). Specifying the file mode should be left to the opener if needed.
msg146714 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-31 16:59
> Updated patch:
> * checks for long overflow
> * raises original exception if opener returns null
> * makes it explicit that "opener" must return an open file descriptor.

This looks good to me. You just need to add a "versionchanged" attribute
in the documentation.
msg146724 - (view) Author: Roundup Robot (python-dev) Date: 2011-10-31 18:34
New changeset 0d64d9ac2b78 by Ross Lagerwall in branch 'default':
Issue #12797: Added custom opener parameter to builtin open() and FileIO.open().
http://hg.python.org/cpython/rev/0d64d9ac2b78
msg146734 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2011-10-31 19:23
Is "an open file descriptor" correct in English? I'd have written "an opened file descriptor" instead (in 5 places).
msg146736 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-10-31 19:35
> Is "an open file descriptor" correct in English? I'd have written "an
> opened file descriptor" instead (in 5 places).

"open" is correct.
For example, you say "the store is open", not "the store is opened": "open" is an adjective, whereas "opened" is the past participe.
See http://www.usingenglish.com/forum/ask-teacher/15771-open-opened-welcome-welcomed.html
msg146737 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2011-10-31 19:40
> I'd have written "an opened file descriptor" instead (in 5 places).

Yes, 'open' is an adjective as well as a verb, and the correct one in 
this context. Something that has been opened, such as a sealed jar or 
envelope, might have been re-closed, but it is no longer the same as 
never-opened.
msg146758 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2011-11-01 04:56
Thanks (and for the English lesson ;-) )
msg147843 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-11-18 09:56
See #13424 for a doc request about this.
msg256854 - (view) Author: Марк Коренберг (mmarkk) * Date: 2015-12-22 19:15
But... os.openat() is still missing... why status is closed() ?!
msg256870 - (view) Author: Eryk Sun (eryksun) * Date: 2015-12-22 21:37
Марк, os.open added dir_fd support in 3.3, which is implemented on POSIX systems by calling openat. The dir_fd parameter is available for many os functions. This is discussed in section 1.5, Files and Directories [1].

It would be nice if we could support dir_fd on Windows as well, but we'd have to bypass the CRT and Windows API to use the native NT API instead, such as NtCreateFile [2]. The kernel has supported opening a file relative to a directory handle since it was release in 1993 (NT 3.1). All named kernel objects are referenced using an OBJECT_ATTRIBUTES [3] data structure. ObjectName -- a path with up to 32768 UTF-16 characters -- is relative to the RootDirectory handle if non-NULL. This is how paths relative to the process working directory are implemented, but changing the working directory isn't thread safe. 

[1]: https://docs.python.org/3/library/os.html#files-and-directories
[2]: https://msdn.microsoft.com/en-us/library/ff566424
[3]: https://msdn.microsoft.com/en-us/library/ff557749
History
Date User Action Args
2015-12-22 21:37:03eryksunsetnosy: + eryksun
messages: + msg256870
2015-12-22 19:15:11mmarkksetnosy: + mmarkk
messages: + msg256854
2011-12-27 17:11:34pitrouunlinkissue12760 superseder
2011-11-18 09:56:00eric.araujosetmessages: + msg147843
2011-11-01 04:56:44rosslagerwallsetstatus: open -> closed
messages: + msg146758

assignee: rosslagerwall
resolution: fixed
stage: needs patch -> resolved
2011-10-31 19:40:28terry.reedysetmessages: + msg146737
2011-10-31 19:35:33neologixsetmessages: + msg146736
2011-10-31 19:23:17amaury.forgeotdarcsetmessages: + msg146734
2011-10-31 18:34:29python-devsetnosy: + python-dev
messages: + msg146724
2011-10-31 16:59:57pitrousetmessages: + msg146714
2011-10-30 19:10:48rosslagerwallsetfiles: + opener_v2.patch

messages: + msg146654
2011-10-30 18:15:04benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg146650
2011-10-30 17:30:39pitrousetmessages: + msg146647
2011-10-30 05:32:14rosslagerwallsetfiles: + opener.patch

messages: + msg146626
2011-10-29 19:21:00pitrousetmessages: + msg146617
2011-10-29 18:33:51rosslagerwallsetmessages: + msg146616
2011-10-29 16:29:46Arfreversetnosy: + Arfrever
2011-10-29 16:23:29pitrousetmessages: + msg146614
2011-10-29 16:16:37rosslagerwallsetmessages: + msg146613
2011-10-29 15:58:21neologixlinkissue12760 superseder
2011-10-29 15:57:50neologixlinkissue12105 superseder
2011-10-29 15:57:13neologixsetmessages: + msg146612
2011-10-18 18:16:54rosslagerwallsetmessages: + msg145849
2011-10-17 17:40:29pitrousetmessages: + msg145741
2011-09-30 06:10:16rosslagerwallsetfiles: + i12797.patch
keywords: + patch
messages: + msg144674
2011-08-28 17:49:26terry.reedysetnosy: + terry.reedy
messages: + msg143112
2011-08-21 12:38:24eric.araujosetnosy: + eric.araujo
2011-08-21 00:01:02ned.deilysetmessages: + msg142588
2011-08-20 23:42:08pitrousetmessages: + msg142582
2011-08-20 23:34:29ned.deilysetnosy: + ned.deily
messages: + msg142581
2011-08-20 22:22:59hayposetmessages: + msg142578
2011-08-20 21:21:16pitrousetmessages: + msg142568
2011-08-20 21:17:16amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg142565
2011-08-20 21:14:30nadeem.vawdasetnosy: + nadeem.vawda
2011-08-20 21:00:02pitroucreate