classification
Title: Windows error code 267 should be mapped to ENOTDIR, not EINVAL
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.2, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, brian.curtin, pitrou, python-dev, tim.golden, vladris
Priority: normal Keywords: patch

Created on 2011-08-21 00:46 by pitrou, last changed 2011-09-01 19:42 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
issue12802.diff vladris, 2011-08-21 03:37 Map Windows error 267 to errno 20 review
issue12802_unittest.diff vladris, 2011-08-24 01:05 Unittest for this case review
winenotdir.patch pitrou, 2011-08-27 15:27 review
issue12802_2.diff vladris, 2011-08-27 15:30 Updated patch including generrmap changes review
Messages (22)
msg142589 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-21 00:46
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).
msg142590 - (view) Author: Vlad Riscutia (vladris) Date: 2011-08-21 03:37
Attached new mapping though I don't know where unit test for this should go...
msg142852 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-23 18:00
I would add it in test_exceptions, since errno mapping is ultimately used by exceptions.
msg142875 - (view) Author: Vlad Riscutia (vladris) Date: 2011-08-24 01:05
Attached unit test.
msg143023 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-26 18:38
Brian, Tim, I'd feel more comfortable if any of you confirmed this isn't a stupid proposal on my part :)
msg143024 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2011-08-26 18:57
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).
msg143026 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-26 19:01
> 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.
msg143027 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2011-08-26 19:04
With that PEP likely to be accepted, I say go ahead with the change for that benefit.
msg143028 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2011-08-26 19:17
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
msg143029 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2011-08-26 19:40
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.
msg143030 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2011-08-26 19:46
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.
msg143053 - (view) Author: Vlad Riscutia (vladris) Date: 2011-08-27 02:33
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
    }
}
msg143060 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2011-08-27 11:11
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.
msg143070 - (view) Author: Vlad Riscutia (vladris) Date: 2011-08-27 14:38
Oh, got it. Interesting. Then should I just add a comment somewhere or should we resolve this as Won't Fix?
msg143071 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-27 14:42
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).
msg143073 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-27 15:00
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.
msg143074 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-27 15:02
Ok, running vcvarsamd64.bat seems to do the trick.
msg143078 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-27 15:27
Here is a new patch.
msg143079 - (view) Author: Vlad Riscutia (vladris) Date: 2011-08-27 15:30
Attached updated patch which extends generrmap.c to allow for easy addition of other error mappings.

Also regenerated errmap.h and unittest.
msg143087 - (view) Author: Vlad Riscutia (vladris) Date: 2011-08-27 20:09
Ah, I see Antoine already attached a patch. I was 3 minutes late :)
msg143346 - (view) Author: Roundup Robot (python-dev) Date: 2011-09-01 19:41
New changeset 385c2ec78f16 by Antoine Pitrou in branch '3.2':
Issue #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 #12802: the Windows error ERROR_DIRECTORY (numbered 267) is now
http://hg.python.org/cpython/rev/d72d5c942232
msg143347 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-09-01 19:42
Committed now. Hopefully it won't break anything!
History
Date User Action Args
2011-09-01 19:42:58pitrousetstatus: open -> closed
versions: - Python 2.7
messages: + msg143347

resolution: fixed
stage: patch review -> resolved
2011-09-01 19:41:13python-devsetnosy: + python-dev
messages: + msg143346
2011-08-27 20:09:19vladrissetmessages: + msg143087
2011-08-27 15:30:20vladrissetfiles: + issue12802_2.diff

messages: + msg143079
2011-08-27 15:27:25pitrousetfiles: + winenotdir.patch

messages: + msg143078
2011-08-27 15:02:08pitrousetmessages: + msg143074
2011-08-27 15:00:38pitrousetmessages: + msg143073
2011-08-27 14:42:27pitrousetmessages: + msg143071
2011-08-27 14:38:30vladrissetmessages: + msg143070
2011-08-27 11:11:26amaury.forgeotdarcsetmessages: + msg143060
2011-08-27 02:33:55vladrissetmessages: + msg143053
2011-08-26 19:46:51amaury.forgeotdarcsetmessages: + msg143030
2011-08-26 19:40:41amaury.forgeotdarcsetmessages: + msg143029
2011-08-26 19:17:34tim.goldensetmessages: + msg143028
2011-08-26 19:04:47brian.curtinsetmessages: + msg143027
2011-08-26 19:01:17pitrousetmessages: + msg143026
2011-08-26 18:57:06brian.curtinsetmessages: + msg143024
2011-08-26 18:38:34pitrousetmessages: + msg143023
components: + Interpreter Core
stage: needs patch -> patch review
2011-08-24 01:05:22vladrissetfiles: + issue12802_unittest.diff

messages: + msg142875
2011-08-23 18:00:42pitrousetmessages: + msg142852
2011-08-21 03:37:06vladrissetfiles: + issue12802.diff

nosy: + vladris
messages: + msg142590

keywords: + patch
2011-08-21 00:46:29pitroucreate