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
Comments
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). |
Attached new mapping though I don't know where unit test for this should go... |
I would add it in test_exceptions, since errno mapping is ultimately used by exceptions. |
Attached unit test. |
Brian, Tim, I'd feel more comfortable if any of you confirmed this isn't a stupid proposal on my part :) |
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 |
With that PEP likely to be accepted, I say go ahead with the change for that benefit. |
Obviously someone's code would break if it were relying on the Unix Speaking for myself, since the exception is a WindowsError with the I say go ahead |
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. |
If you have a copy of Visual Studio, you can see the code of _dosmaperr() in VC/crt/src/dosmap.c. |
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
}
} |
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. |
Oh, got it. Interesting. Then should I just add a comment somewhere or should we resolve this as Won't Fix? |
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). |
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
Microsoft (R) C/C++ Optimizing Compiler Version 15.00.21022.08 for x64 generrmap.c |
Ok, running vcvarsamd64.bat seems to do the trick. |
Here is a new patch. |
Attached updated patch which extends generrmap.c to allow for easy addition of other error mappings. Also regenerated errmap.h and unittest. |
Ah, I see Antoine already attached a patch. I was 3 minutes late :) |
New changeset 385c2ec78f16 by Antoine Pitrou in branch '3.2': New changeset d72d5c942232 by Antoine Pitrou in branch 'default': |
Committed now. Hopefully it won't break anything! |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: