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

io.FileIO and io.open should support openat #57006

Closed
pitrou opened this issue Aug 20, 2011 · 29 comments
Closed

io.FileIO and io.open should support openat #57006

pitrou opened this issue Aug 20, 2011 · 29 comments
Labels
stdlib Python modules in the Lib dir topic-IO type-feature A feature request or enhancement

Comments

@pitrou
Copy link
Member

pitrou commented Aug 20, 2011

BPO 12797
Nosy @terryjreedy, @amauryfa, @pitrou, @vstinner, @benjaminp, @ned-deily, @merwok, @socketpair, @eryksun
Files
  • i12797.patch
  • opener.patch
  • opener_v2.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 2011-11-01.04:56:44.446>
    created_at = <Date 2011-08-20.21:00:02.365>
    labels = ['type-feature', 'library', 'expert-IO']
    title = 'io.FileIO and io.open should support openat'
    updated_at = <Date 2015-12-22.21:37:03.750>
    user = 'https://github.com/pitrou'

    bugs.python.org fields:

    activity = <Date 2015-12-22.21:37:03.750>
    actor = 'eryksun'
    assignee = 'rosslagerwall'
    closed = True
    closed_date = <Date 2011-11-01.04:56:44.446>
    closer = 'rosslagerwall'
    components = ['Library (Lib)', 'IO']
    creation = <Date 2011-08-20.21:00:02.365>
    creator = 'pitrou'
    dependencies = []
    files = ['23272', '23550', '23562']
    hgrepos = []
    issue_num = 12797
    keywords = ['patch']
    message_count = 29.0
    messages = ['142560', '142565', '142568', '142578', '142581', '142582', '142588', '143112', '144674', '145741', '145849', '146612', '146613', '146614', '146616', '146617', '146626', '146647', '146650', '146654', '146714', '146724', '146734', '146736', '146737', '146758', '147843', '256854', '256870']
    nosy_count = 14.0
    nosy_names = ['terry.reedy', 'amaury.forgeotdarc', 'pitrou', 'vstinner', 'nadeem.vawda', 'benjamin.peterson', 'ned.deily', 'eric.araujo', 'Arfrever', 'neologix', 'rosslagerwall', 'socketpair', 'python-dev', 'eryksun']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue12797'
    versions = ['Python 3.3']

    @pitrou
    Copy link
    Member Author

    pitrou commented Aug 20, 2011

    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

    @pitrou pitrou added stdlib Python modules in the Lib dir topic-IO type-feature A feature request or enhancement labels Aug 20, 2011
    @amauryfa
    Copy link
    Member

    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.

    @pitrou
    Copy link
    Member Author

    pitrou commented Aug 20, 2011

    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

    @vstinner
    Copy link
    Member

    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 bpo-4489.

    @ned-deily
    Copy link
    Member

    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.

    @pitrou
    Copy link
    Member Author

    pitrou commented Aug 20, 2011

    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.

    @ned-deily
    Copy link
    Member

    We already have os.openat
    Ah, right. The comment still applies, though, to future documentation of the proposed feature. +1 on it.

    @terryjreedy
    Copy link
    Member

    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.)

    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Sep 30, 2011

    Attached is a patch which adds dirfd= as a keyword argument.

    @pitrou
    Copy link
    Member Author

    pitrou commented Oct 17, 2011

    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?

    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Oct 18, 2011

    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 :-)

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Oct 29, 2011

    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 bpo-12760 and bpo-12105.

    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Oct 29, 2011

    What would you envisage the API for the custom opener to look like?

    @pitrou
    Copy link
    Member Author

    pitrou commented Oct 29, 2011

    What would you envisage the API for the custom opener to look like?

    The same as os.open(), I would say.

    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Oct 29, 2011

    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")
    """

    @pitrou
    Copy link
    Member Author

    pitrou commented Oct 29, 2011

    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.

    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Oct 30, 2011

    The attached patch adds the opener keyword + tests.

    @pitrou
    Copy link
    Member Author

    pitrou commented Oct 30, 2011

    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!

    @benjaminp
    Copy link
    Contributor

    Also, the documentation should indicate what exactly is supposed to be returned by "opener".

    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Oct 30, 2011

    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.

    @pitrou
    Copy link
    Member Author

    pitrou commented Oct 31, 2011

    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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 31, 2011

    New changeset 0d64d9ac2b78 by Ross Lagerwall in branch 'default':
    Issue bpo-12797: Added custom opener parameter to builtin open() and FileIO.open().
    http://hg.python.org/cpython/rev/0d64d9ac2b78

    @amauryfa
    Copy link
    Member

    Is "an open file descriptor" correct in English? I'd have written "an opened file descriptor" instead (in 5 places).

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Oct 31, 2011

    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

    @terryjreedy
    Copy link
    Member

    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.

    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Nov 1, 2011

    Thanks (and for the English lesson ;-) )

    @rosslagerwall rosslagerwall mannequin closed this as completed Nov 1, 2011
    @rosslagerwall rosslagerwall mannequin self-assigned this Nov 1, 2011
    @merwok
    Copy link
    Member

    merwok commented Nov 18, 2011

    See bpo-13424 for a doc request about this.

    @socketpair
    Copy link
    Mannequin

    socketpair mannequin commented Dec 22, 2015

    But... os.openat() is still missing... why status is closed() ?!

    @eryksun
    Copy link
    Contributor

    eryksun commented Dec 22, 2015

    Марк, 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.

    @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
    stdlib Python modules in the Lib dir topic-IO type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    8 participants