search

haskell / win32

Haskell support for the Win32 API
http://hackage.haskell.org/package/Win32
Other
97 stars 63 forks source link

Call SetLastError before maperrno #60

Closed egor-tensin closed 8 years ago

egor-tensin commented 8 years ago

I've discovered that failWith doesn't work perfectly with IOError. In particular, it appears that it doesn't provide proper IOErrorType. GetLastError()-to-errno conversion aside, one issue seems to be that some WinAPI functions don't actually call SetLastError, returning an error code instead. In those cases, when maperrno is called, it converts a junk value returned by GetLastError. I added a call to SetLastError just before the call to maperrno so that it converts the actual error code returned by an API call.

I bumped into this while trying to detect when regOpenKey fails because of the key missing. I used isDoesNotExistError for this, only to discover that it doesn't work. I've created two repositories to demonstrate the issue and the fix:

(The code is identical in both. I wanted to somehow make two source files instead of the full-fledges repos, but my cabal/stack-fu is not on par.)

I realize that this fix is lacking proper verification and testing; I'm just hoping somebody more competent will take a look.

Mistuke commented 8 years ago

Thanks for the report, Although I'm not quite sure about using SetLastError here. I think we should instead use maperrno_func to convert the error instead of maperrno and pass it the error code directly.

That should be much faster than going through the TLS for an error we already have. However I wonder If this error code passed to the function is always filled in. Have you checked to see with an API call that does call SetLastError?

egor-tensin commented 8 years ago

I have, see e.g. https://github.com/egor-tensin/win32-issue-setlasterror. About the error code not being filled in, do you mean that somebody might pass some kind of a dumb value to failWith? According to a quick grep, nobody does.

BTW, I think that calling SetLastError prior to maperrno is beneficial for one more reason: FormatMessage (used in getErrorMessage) might also call SetLastError, erasing the value before the call to maperrno.

I agree that some kind of an error code conversion function would be much better than the current mechanism (which is quite error-prone), I just tried to introduce a minimal fix.

Mistuke commented 8 years ago

Sorry, I don't completely follow, I don't see anything in that repository except a test that catches a file not found exception?

What I meant before is, why call SetLastError at all when all we do with the error code is pass it to maperrno_func anyway in maperrno. We might as well directly call maperrno_func instead of maperrno passing it the error code directly. getErrorMessage also takes an explicit error code.

Unless I am misunderstanding something?

egor-tensin commented 8 years ago

Yes, right. Please disregard my previous inquiries. The reason for my confusion was that I haven't even noticed that maperrno is just a tiny wrapper around maperrno_func, which is of course much better. Can you please take a look at the current version? Seems like it properly solves every problem.

I was rambling about getErrorMessage because the underlying FormatMessageW can call SetLastError if used incorrectly, which would erase the value to be later used by maperrno, but the current version renders that concern obsolete.

Mistuke commented 8 years ago

Great! thanks!. Added a minor nit. Could you update and add a small entry to the changelog and then I'll merge.

egor-tensin commented 8 years ago

Added a changelog entry.

Mistuke commented 8 years ago

Thanks for the report and fix!