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

cmake: set HAVE_GETADDRINFO_THREADSAFE on Windows #9727

Closed
wants to merge 4 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 13, 2022

autotools enables this configuration option unconditionally for Windows 1. Do the same in CMake.

The above will make this work for all reasonably recent environments. The logic present in lib/config-win32.h 2 has the following exceptions which we did not cover in this CMake update:

  • Builds targeting Windows 2000 and earlier
  • MS Visual C++ 5.0 (1997) and earlier

Defining HAVE_GETADDRINFO_THREADSAFE without
HAVE_GETADDRINFO results in a broken build:
https://ci.appveyor.com/project/curlorg/curl/builds/45062422/job/tjyv75wxfdhaledj#L357
Add condition to ensure this doesn't happen. (It would probably be nicer to check for this in C).

Closes #xxxx

Footnotes

  1. curl/m4/curl-functions.m4

    Lines 2067 to 2070 in 68fa9bf

    if test "$tst_tsafe_getaddrinfo" = "unknown" &&
    test "$curl_cv_native_windows" = "yes"; then
    tst_tsafe_getaddrinfo="yes"
    fi

  2. curl/lib/config-win32.h

    Lines 511 to 528 in 68fa9bf

    /* Availability of freeaddrinfo, getaddrinfo, and if_nametoindex
    functions is quite convoluted, compiler dependent and even build target
    dependent. */
    #if defined(HAVE_WS2TCPIP_H)
    # if defined(__POCC__)
    # define HAVE_FREEADDRINFO 1
    # define HAVE_GETADDRINFO 1
    # define HAVE_GETADDRINFO_THREADSAFE 1
    # elif defined(_WIN32_WINNT) && (_WIN32_WINNT >= 0x0501)
    # define HAVE_FREEADDRINFO 1
    # define HAVE_GETADDRINFO 1
    # define HAVE_GETADDRINFO_THREADSAFE 1
    # elif defined(_MSC_VER) && (_MSC_VER >= 1200)
    # define HAVE_FREEADDRINFO 1
    # define HAVE_GETADDRINFO 1
    # define HAVE_GETADDRINFO_THREADSAFE 1
    # endif
    #endif

@vszakats vszakats added cmake Windows Windows-specific labels Oct 13, 2022
vszakats added a commit to vszakats/curl that referenced this pull request Oct 13, 2022
autotools enables this configuration option unconditionally for Windows
[^1]. Do the same in CMake.

The above will make this work for all reasonably recent environments.
The logic present in `lib/config-win32.h` [^2] has the following
exceptions which we did not cover in this CMake update:

- Builds targeting Windows 2000 and earlier
- MS Visual C++ 5.0 (1997) and earlier

[^1]: https://github.com/curl/curl/blob/68fa9bf3f5d7b4fcbb57619f70cb4aabb79a51f6/m4/curl-functions.m4#L2067-L2070

[^2]: https://github.com/curl/curl/blob/68fa9bf3f5d7b4fcbb57619f70cb4aabb79a51f6/lib/config-win32.h#L511-L528

Closes curl#9727
vszakats added 2 commits Oct 14, 2022
autotools enables this configuration option unconditionally for Windows
[^1]. Do the same in CMake.

The above will make this work for all reasonably recent environments.
The logic present in `lib/config-win32.h` [^2] has the following
exceptions which we did not cover in this CMake update:

- Builds targeting Windows 2000 and earlier
- MS Visual C++ 5.0 (1997) and earlier

[^1]: https://github.com/curl/curl/blob/68fa9bf3f5d7b4fcbb57619f70cb4aabb79a51f6/m4/curl-functions.m4#L2067-L2070

[^2]: https://github.com/curl/curl/blob/68fa9bf3f5d7b4fcbb57619f70cb4aabb79a51f6/lib/config-win32.h#L511-L528

Closes curl#9727
@vszakats
Copy link
Member Author

vszakats commented Oct 14, 2022

I did a patch that adds a C-level solution for this, do we want it?

Date:   2022-10-14 14:36:21 +0000

    sync conditions for Curl_getaddrinfo_ex() def and use
    
    `Curl_getaddrinfo_ex()` gets _defined_ with `HAVE_GETADDRINFO` set.
    But, `hostip4.c` _used_ it with `HAVE_GETADDRINFO_THREADSAFE` set alone.
    It meant a build with the latter, but without the former flag could
    result in calling this function but not defining it, and failing to
    link.
    
    Patch this by adding an extra check for `HAVE_GETATTRINFO` around
    the call.
    
    Before this patch, build systems prevented this condition. Now they
    don't need to.

diff --git a/lib/hostip4.c b/lib/hostip4.c
index 1dd54e879..df4f2c89b 100644
--- a/lib/hostip4.c
+++ b/lib/hostip4.c
@@ -125,14 +125,15 @@ struct Curl_addrinfo *Curl_getaddrinfo(struct Curl_easy *data,
 struct Curl_addrinfo *Curl_ipv4_resolve_r(const char *hostname,
                                           int port)
 {
-#if !defined(HAVE_GETADDRINFO_THREADSAFE) && defined(HAVE_GETHOSTBYNAME_R_3)
+#if !(defined(HAVE_GETADDRINFO) && defined(HAVE_GETADDRINFO_THREADSAFE)) && \
+   defined(HAVE_GETHOSTBYNAME_R_3)
   int res;
 #endif
   struct Curl_addrinfo *ai = NULL;
   struct hostent *h = NULL;
   struct hostent *buf = NULL;
 
-#if defined(HAVE_GETADDRINFO_THREADSAFE)
+#if defined(HAVE_GETADDRINFO) && defined(HAVE_GETADDRINFO_THREADSAFE)
   struct addrinfo hints;
   char sbuf[12];
   char *sbufptr = NULL;
@@ -280,14 +281,16 @@ struct Curl_addrinfo *Curl_ipv4_resolve_r(const char *hostname,
     h = NULL; /* set return code to NULL */
     free(buf);
   }
-#else /* HAVE_GETADDRINFO_THREADSAFE || HAVE_GETHOSTBYNAME_R */
+#else /* (HAVE_GETADDRINFO && HAVE_GETADDRINFO_THREADSAFE) ||
+          HAVE_GETHOSTBYNAME_R */
   /*
    * Here is code for platforms that don't have a thread safe
    * getaddrinfo() nor gethostbyname_r() function or for which
    * gethostbyname() is the preferred one.
    */
   h = gethostbyname((void *)hostname);
-#endif /* HAVE_GETADDRINFO_THREADSAFE || HAVE_GETHOSTBYNAME_R */
+#endif /* (HAVE_GETADDRINFO && HAVE_GETADDRINFO_THREADSAFE) ||
+           HAVE_GETHOSTBYNAME_R */
 
   if(h) {
     ai = Curl_he2ai(h, port);

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

Successfully merging this pull request may close these issues.

None yet

1 participant