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

Jump to bottom

smb: replace CURL_WIN32 with WIN32 #9701

Closed
wants to merge 1 commit into from
Closed

smb: replace CURL_WIN32 with WIN32 #9701

wants to merge 1 commit into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 11, 2022 •
edited

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

@vszakats
smb: replace CURL_WIN32 with WIN32
f937cb3 
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

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

@vszakats
Copy link
Member Author

vszakats commented Oct 11, 2022 •
edited

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 •
edited

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

@vszakats vszakats mentioned this pull request Oct 11, 2022
Closed
@vszakats vszakats closed this in 28edcbe Oct 11, 2022
@vszakats vszakats deleted the smbw32 branch October 11, 2022 21:10
obonaventure pushed a commit to mptcp-apps/curl that referenced this pull request Oct 12, 2022
@vszakats @obonaventure
smb: replace CURL_WIN32 with WIN32
2f1bc3b 
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
Reviewers

@MarcelRaad MarcelRaad MarcelRaad approved these changes

Assignees
No one assigned
Labels
SMB Windows Windows-specific
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vszakats @MarcelRaad