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

Support IPv6 adresses without getaddrinfo #4662

Closed
wants to merge 2 commits into from

Conversation

@MarcelRaad
Copy link
Member

MarcelRaad commented Nov 30, 2019

This makes the autotools build behave like the CMake build in this regard: support IPv6 addresses (ENABLE_IPV6) by default as long as AF_INET6 and sockaddr_in6 are available. getaddrinfo is only needed for DNS resolution (CURLRES_IPV6).

This also fixes the CMake build with ENABLE_IPV6 set to the default ON, ENABLE_THREADED_RESOLVER set to OFF, and no getaddrinfo available. The theaded resolver code guards the calls to getaddrinfo with HAVE_GETADDRINFO, but the synchronous resolver code assumes that CURLRES_IPV6 implies HAVE_GETADDRINFO, which it did only implicitly when using the autotools build.

Tested with classic MinGW, which disables getaddrinfo by default by targeting Windows 2000, and both the threaded and synchronous resolvers.

MarcelRaad added 2 commits Nov 26, 2019
Also, use `CURLRES_IPV6` only for actual DNS resolution, not for IPv6
address support. This makes it possible to connect to IPv6 literals by
setting `ENABLE_IPV6` even without `getaddrinfo` support. It also fixes
the CMake build when using the synchronous resolver without
`getaddrinfo` support.

Closes
This makes it possible to recognize and connect to literal IPv6
addresses when `getaddrinfo` is not available, which is already the
case for the CMake build. This affects e.g. classic MinGW because it
still targets Windows 2000 by default, where `getaddrinfo` is not
available, but general IPv6 support is.

Instead of checking for `getaddrinfo`, check for `sockaddr_in6` as the
CMake build does.

Closes
@bagder
Copy link
Member

bagder commented Dec 1, 2019

Can you elaborate on why you want this supported? To me it sounds like we mostly open up for a grey in-between support and not support situation instead of having a clear binary state. Why do we want a curl build that knows about IPv6 but can't resolve IPv6 host names?

@MarcelRaad
Copy link
Member Author

MarcelRaad commented Dec 1, 2019

What I wanted to do was to unify the behavior for the different build systems because I noticed different behavior depending on which one I use with MinGW and older Visual Studio. The CMake, winbuild, DOS, and Visual Studio project builds (and maybe others) don't take getaddrinfo into account for IPv6 support. Only the autotools build does.

I just couldn't find any point in forbidding the user to connect to literal IPv6 addresses at build time if not explicitly disabled with --disable-ipv6. Also, this increases test coverage for these systems. This is why this made more sense to me than trying to change all other build systems to match the current autotools behavior.

@bagder
Copy link
Member

bagder commented Dec 1, 2019

Ok, that makes sense - thanks for the explanation. I'm not overly worried that this will cause new support problems since I suspect this is still a rather niche problem. Most users will still have getaddrinfo...

MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Dec 3, 2019
This makes it possible to recognize and connect to literal IPv6
addresses when `getaddrinfo` is not available, which is already the
case for the CMake build. This affects e.g. classic MinGW because it
still targets Windows 2000 by default, where `getaddrinfo` is not
available, but general IPv6 support is.

Instead of checking for `getaddrinfo`, check for `sockaddr_in6` as the
CMake build does.

Closes curl#4662
@MarcelRaad MarcelRaad closed this in 67a08dc Dec 3, 2019
@MarcelRaad MarcelRaad deleted the MarcelRaad:ipv6_without_getaddrinfo branch Dec 3, 2019
@bagder
Copy link
Member

bagder commented Dec 4, 2019

I'm pretty sure this caused three test cases to fail when c-ares is used: https://travis-ci.org/curl/curl/jobs/620363654#L5046

@MarcelRaad
Copy link
Member Author

MarcelRaad commented Dec 4, 2019

Thanks, I'll look into that later today! c-ares was the only backend I didn't test locally. Travis was all green though when I merged this, except for the broken macOS jobs, including the --enable-ares one: https://travis-ci.org/curl/curl/builds/619036114

@lock lock bot locked as resolved and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.