Skip to content
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

smb: replace CURL_WIN32 with WIN32 #9701

Closed
wants to merge 1 commit into from
Closed

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 11, 2022

PR #9255 aimed to fix a Cygwin/MSYS issue. It used the CURL_WIN32 macro, but that one is not defined here, while compiling curl itself. This patch changes this to WIN32, assuming this was the original intent.

Regression from 1c52e8a.

Ref: #8220

Closes #9701

/cc @orgads, @MarcelRaad

PR curl#9244 aimed to fix a Cygwin/MSYS issue. It used the `CURL_WIN32`
macro, but that one is never defined while compiling curl itself. This
patch changes this to `WIN32`, assuming this was the original intent.

Regression from 1c52e8a.

Ref: curl#8220
Ref: curl#9255

Closes #xxxx
@vszakats vszakats added Windows Windows-specific SMB labels Oct 11, 2022
@MarcelRaad
Copy link
Member

MarcelRaad commented Oct 11, 2022

The PR 9244 in the commit message should be 9255, right?

@vszakats
Copy link
Member Author

vszakats commented Oct 11, 2022

Maybe this condition could be simplified to never depend on HAVE_PROCESS_H on Windows (which is defined by CMake/config-win32.h, but not by autotools), and always use the lowest-level Win32 API instead: GetCurrentProcessId(). Untested:

#ifdef WIN32
#define getpid GetCurrentProcessId
#elif defined(HAVE_PROCESS_H)
#include <process.h>
#endif

diff:

diff --git a/lib/smb.c b/lib/smb.c
index a62e85814..42f22f189 100644
--- a/lib/smb.c
+++ b/lib/smb.c
@@ -30,13 +30,10 @@
 
 #define BUILDING_CURL_SMB_C
 
-#ifdef HAVE_PROCESS_H
-#include <process.h>
-#ifdef CURL_WINDOWS_APP
+#ifdef WIN32
 #define getpid GetCurrentProcessId
-#elif defined(WIN32)
-#define getpid _getpid
-#endif
+#elif defined(HAVE_PROCESS_H)
+#include <process.h>
 #endif
 
 #include "smb.h"

Opinions?

@vszakats
Copy link
Member Author

vszakats commented Oct 11, 2022

@MarcelRaad: You're right, I mis-typed that one.

@vszakats vszakats closed this in 28edcbe Oct 11, 2022
@vszakats vszakats deleted the smbw32 branch Oct 11, 2022
obonaventure pushed a commit to mptcp-apps/curl that referenced this pull request Oct 12, 2022
PR curl#9255 aimed to fix a Cygwin/MSYS issue (curl#8220). It used the
`CURL_WIN32` macro, but that one is not defined here, while compiling
curl itself. This patch changes this to `WIN32`, assuming this was the
original intent.

Regression from 1c52e8a

Reviewed-by: Marcel Raad

Closes curl#9701
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SMB Windows Windows-specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants