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

Windows error code 267 should be mapped to ENOTDIR, not EINVAL #57011

Closed
pitrou opened this issue Aug 21, 2011 · 22 comments
Closed

Windows error code 267 should be mapped to ENOTDIR, not EINVAL #57011

pitrou opened this issue Aug 21, 2011 · 22 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@pitrou
Copy link
Member

pitrou commented Aug 21, 2011

BPO 12802
Nosy @amauryfa, @pitrou, @tjguk, @briancurtin
Files
  • issue12802.diff: Map Windows error 267 to errno 20
  • issue12802_unittest.diff: Unittest for this case
  • winenotdir.patch
  • issue12802_2.diff: Updated patch including generrmap changes
  • 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-09-01.19:42:58.899>
    created_at = <Date 2011-08-21.00:46:29.523>
    labels = ['interpreter-core', 'type-bug']
    title = 'Windows error code 267 should be mapped to ENOTDIR, not EINVAL'
    updated_at = <Date 2011-09-01.19:42:58.898>
    user = 'https://github.com/pitrou'

    bugs.python.org fields:

    activity = <Date 2011-09-01.19:42:58.898>
    actor = 'pitrou'
    assignee = 'none'
    closed = True
    closed_date = <Date 2011-09-01.19:42:58.899>
    closer = 'pitrou'
    components = ['Interpreter Core']
    creation = <Date 2011-08-21.00:46:29.523>
    creator = 'pitrou'
    dependencies = []
    files = ['22969', '23026', '23053', '23054']
    hgrepos = []
    issue_num = 12802
    keywords = ['patch']
    message_count = 22.0
    messages = ['142589', '142590', '142852', '142875', '143023', '143024', '143026', '143027', '143028', '143029', '143030', '143053', '143060', '143070', '143071', '143073', '143074', '143078', '143079', '143087', '143346', '143347']
    nosy_count = 6.0
    nosy_names = ['amaury.forgeotdarc', 'pitrou', 'tim.golden', 'brian.curtin', 'python-dev', 'vladris']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue12802'
    versions = ['Python 3.2', 'Python 3.3']

    @pitrou
    Copy link
    Member Author

    pitrou commented Aug 21, 2011

    Windows error 3 is returned when a directory path doesn't exist, but 267 is returned when an existing path is not a directory. This corresponds straight to the definition of ENOTDIR, but Python translates it to EINVAL:

    >>> os.listdir("xx")
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    WindowsError: [Error 3] The system cannot find the path specified: 'xx\\*.*'
    
    >>> os.listdir("LICENSE")
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    WindowsError: [Error 267] The directory name is invalid: 'LICENSE\\*.*'
    
    >>> os.chdir("LICENSE")
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    WindowsError: [Error 267] The directory name is invalid: 'LICENSE'
    
    >>> e = sys.last_value
    >>> e.errno
    22
    >>> errno.errorcode[22]
    'EINVAL'

    In PC/errmap.h, no Windows error code is translated to ENOTDIR (errno 20).

    @pitrou pitrou added the type-bug An unexpected behavior, bug, or error label Aug 21, 2011
    @vladris
    Copy link
    Mannequin

    vladris mannequin commented Aug 21, 2011

    Attached new mapping though I don't know where unit test for this should go...

    @pitrou
    Copy link
    Member Author

    pitrou commented Aug 23, 2011

    I would add it in test_exceptions, since errno mapping is ultimately used by exceptions.

    @vladris
    Copy link
    Mannequin

    vladris mannequin commented Aug 24, 2011

    Attached unit test.

    @pitrou
    Copy link
    Member Author

    pitrou commented Aug 26, 2011

    Brian, Tim, I'd feel more comfortable if any of you confirmed this isn't a stupid proposal on my part :)

    @pitrou pitrou added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Aug 26, 2011
    @briancurtin
    Copy link
    Member

    I could see how they'd use EINVAL, but to me ENOTDIR makes more sense here. However, I'm not sure if anyone is depending on this (or what they could depend on it for).

    @pitrou
    Copy link
    Member Author

    pitrou commented Aug 26, 2011

    I could see how they'd use EINVAL, but to me ENOTDIR makes more sense
    here. However, I'm not sure if anyone is depending on this (or what
    they could depend on it for).

    Right now I'm not sure, but if PEP-3151 is accepted it will make much
    more sense to get a NotADirectoryError.

    @briancurtin
    Copy link
    Member

    With that PEP likely to be accepted, I say go ahead with the change for that benefit.

    @tjguk
    Copy link
    Member

    tjguk commented Aug 26, 2011

    Obviously someone's code would break if it were relying on the Unix
    errno only in a Windows-only situation to determine the situation of
    opening a directory which isn't one. But that combination of events
    doesn't seem terribly likely.

    Speaking for myself, since the exception is a WindowsError with the
    winerror attribute prominent, [Error 267] I'd be far more likely to be
    trapping that.

    I say go ahead

    @amauryfa
    Copy link
    Member

    Note that this file is not written by hand. It's generated by PC/generrmap.c, which uses the _dosmaperr() function provided by the msvcrt.

    If we want to modify it, this should be clearly marked somewhere.

    @amauryfa
    Copy link
    Member

    If you have a copy of Visual Studio, you can see the code of _dosmaperr() in VC/crt/src/dosmap.c.
    Otherwise the Google query "inurl:dosmap.c" returns some online copies of this file.

    @vladris
    Copy link
    Mannequin

    vladris mannequin commented Aug 27, 2011

    I wasn't aware this is an auto-generated file. I can add a comment but looking at it, it seems we auto-generate this file just to save a call to _dosmaperr. I would refactor the whole function to call _dosmaperr first then if result is still EINVAL, tweak with custom switch case. The way I see it, this looks like premature optimization since OS error shouldn't be on a hot code path, meaning an application should be able to live with an extra CRT function call on such exceptions. I'm willing to implement this if there are no objections. Something like:

    errno = _dosmaperr(err)
    if (EINVAL == errno)
    {
        switch (err)
        {
        // Our tweaks
        }
    }

    @amauryfa
    Copy link
    Member

    Unfortunately, it won't work. _dosmaperr() is not exported by msvcrt.dll, it is only available when you link against the static version of the C runtime.

    @vladris
    Copy link
    Mannequin

    vladris mannequin commented Aug 27, 2011

    Oh, got it. Interesting. Then should I just add a comment somewhere or should we resolve this as Won't Fix?

    @pitrou
    Copy link
    Member Author

    pitrou commented Aug 27, 2011

    We could add a special case to generrmap.c (but how can I compile and execute this file? it doesn't seem to be part of the project files).

    @pitrou
    Copy link
    Member Author

    pitrou commented Aug 27, 2011

    Ok, apparently I can use errmap.mak, except that I get the following error:

    Z:\default\PC>nmake errmap.mak

    Microsoft (R) Program Maintenance Utility Version 9.00.21022.08
    Copyright (C) Microsoft Corporation. All rights reserved.

        cl  generrmap.c
    

    Microsoft (R) C/C++ Optimizing Compiler Version 15.00.21022.08 for x64
    Copyright (C) Microsoft Corporation. All rights reserved.

    generrmap.c
    generrmap.c(1) : fatal error C1034: stdio.h: no include path set
    NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio 9.0\
    VC\bin\amd64\cl.EXE"' : return code '0x2'
    Stop.

    @pitrou
    Copy link
    Member Author

    pitrou commented Aug 27, 2011

    Ok, running vcvarsamd64.bat seems to do the trick.

    @pitrou
    Copy link
    Member Author

    pitrou commented Aug 27, 2011

    Here is a new patch.

    @vladris
    Copy link
    Mannequin

    vladris mannequin commented Aug 27, 2011

    Attached updated patch which extends generrmap.c to allow for easy addition of other error mappings.

    Also regenerated errmap.h and unittest.

    @vladris
    Copy link
    Mannequin

    vladris mannequin commented Aug 27, 2011

    Ah, I see Antoine already attached a patch. I was 3 minutes late :)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 1, 2011

    New changeset 385c2ec78f16 by Antoine Pitrou in branch '3.2':
    Issue bpo-12802: the Windows error ERROR_DIRECTORY (numbered 267) is now
    http://hg.python.org/cpython/rev/385c2ec78f16

    New changeset d72d5c942232 by Antoine Pitrou in branch 'default':
    Issue bpo-12802: the Windows error ERROR_DIRECTORY (numbered 267) is now
    http://hg.python.org/cpython/rev/d72d5c942232

    @pitrou
    Copy link
    Member Author

    pitrou commented Sep 1, 2011

    Committed now. Hopefully it won't break anything!

    @pitrou pitrou closed this as completed Sep 1, 2011
    @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
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants