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

curl_setup_once: Remove ERRNO/SET_ERRNO macros #1589

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@jay
Member

jay commented Jun 19, 2017

Prior to this change (SET_)ERRNO mapped to GetLastError/SetLastError
for Win32 and regular errno otherwise.

I reviewed the code and found no justifiable reason for conflating errno
on WIN32 with GetLastError/SetLastError. All Win32 CRTs support errno,
and any Win32 multithreaded CRT supports thread-local errno.

Fixes #895

@mention-bot

This comment has been minimized.

mention-bot commented Jun 19, 2017

@jay, thanks for your PR! By analyzing the history of the files in this pull request, we identified @aYasuharuYamada, @bagder and @yangtse to be potential reviewers.

@coveralls

This comment has been minimized.

coveralls commented Jun 19, 2017

Coverage Status

Coverage decreased (-73.4%) to 0.0% when pulling 7be1ff7 on jay:remove_ERRNO_macro into 65ca030 on curl:master.

@bagder

This comment has been minimized.

Member

bagder commented Jun 19, 2017

Seems you forgot something in lib/memdebug.c causing build errors.

@jay jay force-pushed the jay:remove_ERRNO_macro branch from 7be1ff7 to 1b0e256 Jun 19, 2017

@jay

This comment has been minimized.

Member

jay commented Jun 19, 2017

Seems you forgot something in lib/memdebug.c causing build errors.

fixed, thanks. tested with CPPFLAGS=-DMEMDEBUG_LOG_SYNC and --enable-debug

@coveralls

This comment has been minimized.

coveralls commented Jun 19, 2017

Coverage Status

Coverage decreased (-73.8%) to 0.0% when pulling 1b0e256 on jay:remove_ERRNO_macro into b778ae4 on curl:master.

@bagder

This comment has been minimized.

Member

bagder commented Jun 20, 2017

strerror.c: In function 'Curl_strerror':
strerror.c:634:3: error: unknown type name 'DWORD'
strerror.c:640:3: error: implicit declaration of function 'GetLastError' [-Werror=implicit-function-declaration]
strerror.c:640:3: warning: nested extern declaration of 'GetLastError' [-Wnested-externs]
strerror.c:730:5: error: implicit declaration of function 'SetLastError' [-Werror=implicit-function-declaration]
strerror.c:730:5: warning: nested extern declaration of 'SetLastError' [-Wnested-externs]
curl_setup_once: Remove ERRNO/SET_ERRNO macros
Prior to this change (SET_)ERRNO mapped to GetLastError/SetLastError
for Win32 and regular errno otherwise.

I reviewed the code and found no justifiable reason for conflating errno
on WIN32 with GetLastError/SetLastError. All Win32 CRTs support errno,
and any Win32 multithreaded CRT supports thread-local errno.

Fixes #895

@jay jay force-pushed the jay:remove_ERRNO_macro branch from 1b0e256 to e8ced6c Jun 26, 2017

@jay

This comment has been minimized.

Member

jay commented Jun 26, 2017

Ok I think I got them all. The functions in strerror.c can modify errno so it's saved beforehand, and now that the Windows API error code is separate from errno it should be preserved as well. I had changed the code to do that but did not guard one of the sections as Windows, oops. I've since made some more robust changes to correct it, we'll see what the CI says..

@coveralls

This comment has been minimized.

coveralls commented Jun 26, 2017

Coverage Status

Coverage decreased (-0.01%) to 73.8% when pulling e8ced6c on jay:remove_ERRNO_macro into 922f800 on curl:master.

@jay jay closed this in af02162 Jul 10, 2017

@jay jay deleted the jay:remove_ERRNO_macro branch Jul 10, 2017

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 10, 2017

@jay Seems like you pushed an older version of this - the changes to memdebug.c and strerror.c are missing in af02162.

@gvanem

This comment has been minimized.

Member

gvanem commented Jul 10, 2017

@MarcelRaad Confirmed. Patch:

--- a/memdebug.c 2017-07-10 11:00:17
+++ b/memdebug.c 2017-07-10 10:27:25
@@ -147,7 +147,7 @@
                 source, line, func);
         fflush(logfile); /* because it might crash now */
       }
-      errno = ENOMEM;
+      SET_ERRNO(ENOMEM);
       return TRUE; /* RETURN ERROR! */
     }
     else

jay added a commit to jay/curl that referenced this pull request Jul 11, 2017

strerror: Preserve Windows error code in some functions
This is a follow-up to af02162 which removed (SET_)ERRNO macros. That
commit was an earlier draft that I committed by mistake, which was then
remedied by a5834e5 and e909de6, and now this commit. With this commit
there is now no difference between the current code and the changes that
were approved in the final draft.

Thanks-to: Max Dymond, Marcel Raad, Daniel Stenberg, Gisle Vanem
Ref: curl#1589

@jay jay restored the jay:remove_ERRNO_macro branch Jul 11, 2017

@jay

This comment has been minimized.

Member

jay commented Jul 11, 2017

Thanks guys, I had mistakenly committed an older draft because my repos were out of sync. I just committed the rest in c5e87fd, so there is now no difference between the current code and the changes that were approved in the final draft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment