search

imatix / gsl

iMatix GSL code generator
http://www.imatix.com
GNU General Public License v3.0
539 stars 107 forks source link

Problem: residual errno side effect causes failures. #86

Closed evoskuil closed 9 years ago

evoskuil commented 9 years ago

It's a bit challenging to follow the side effects of global errno state. This change only ensures that in the case of open_dir an uncleared error state doesn't corrupt the current call. Without this change there are at least two distinct failure scenarios in WIN32.

jschultz commented 9 years ago

Is this change going to change functionality? I'm not 100% clear, but does a successful function call set errno to zero or just leave it as it was. If the latter, then it would be better to save and restore the errno.

Also, perhaps this is something that needs to be done uniformly, not just for dir_open?

Finally, if the function 'directory.create' doesn't accept an error parameter, that is presumably an oversight, and it should be made to accept one. Or is there some technical reason why it can't?

Cheers, Jonathan

On 28/03/15 20:07, Pieter Hintjens wrote:

Merged #86 https://github.com/imatix/gsl/pull/86.

— Reply to this email directly or view it on GitHub https://github.com/imatix/gsl/pull/86#event-267515573.

evoskuil commented 9 years ago

By fixing a logical break it does change behavior, but it certainly should not break any correct behavior. There are places where dir_open is called and following the call the value of errno is tested as indication of success/fail of that call. A residual errno (which in at least two places appears to be set as an an unintended side effect, since the value is never tested) passes through dir_open and causes the result to appear failed where it has actually succeeded.

I looked at a couple of different ways to fix it, including preventing the other calls from unintentionally setting errno. Certain Windows API calls are setting it despite the fact that they do not document this behavior. Suppressing it in those cases would prevent the specific errors, but there could be others. The failures I encountered will occur in any case where any call leaves errno set and then dir_open is called. I encountered two such calls, but there could be any number.

As a result the only way to ensure the dir_open failure does not continue to crop up as the result of making other calls that don't clear errno is to clear it at the beginning of dir_open. This is the only rational behavior for dir_open, since it is clearly expected that errno will be set on error and not set if no error. This is a safe change, especially in comparison to the alternatives of (1) changing the calling functions to ignore errno or (2) changing all other functions so that they clear errno after possibly setting it.

Restoring it after clearing it (internal to dir_open) wouldn't prevent the breaks. The caller would have to save it, call dir_open and restore it after making the call. That's a pretty absurd calling convention and is unnecessary as the code exists, since in any affected call to dir_open, with errno set prior, would be causing the failure that I encountered.

I terms of a uniform solution, that would be nice. I looked at it. But there is a very large amount of affected code, errno is a global, and there is no regression test suite. As such making a uniform change would be extremely challenging.

The error parameter is a distinct issue. I only corrected the documentation but I don't see any reason why the API couldn't be made consistent.