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

win32: drop support for WinSock version 1, require version 2 #5854

Closed
wants to merge 2 commits into from

Conversation

@mback2k
Copy link
Member

@mback2k mback2k commented Aug 25, 2020

New PR content:

IPv6, telnet and now also the multi API require WinSock
version 2 which is available starting with Windows 95.

Therefore we think it is time to drop support for version 1.


Original PR content:

While digging through the WinSock setup in the curl source code, I found out that there is actually code to handle the differences between WinSock version 1 and 2. Therefore the changes introduced with #5634 should also still support compilation against older WinSock versions.

An alternative to repeating the same check in every #if statement would be to define an internal variable inside multi.c or multihandle.h and just #ifdef on that one instead of just USE_WINSOCK.

--

Update the ifdef-jungle to check for WinSock version 2.

Follow up to #5634

@mback2k mback2k requested review from bagder and jay Aug 25, 2020
@mback2k mback2k self-assigned this Aug 25, 2020
@mback2k mback2k added the Windows label Aug 25, 2020
@jay
Copy link
Member

@jay jay commented Aug 26, 2020

Is there really a reason to continue supporting Winsock 1

@mback2k
Copy link
Member Author

@mback2k mback2k commented Aug 26, 2020

I don't think there is a good reason to still support it, but can we just drop support for it without deprecating it first?

@mback2k
Copy link
Member Author

@mback2k mback2k commented Aug 26, 2020

I think a good alternative would be to define USE_WINSOCK_1 and USE_WINSOCK_2 depending on the version and check for that.

@mback2k mback2k force-pushed the mback2k:multi-winsock2 branch from aa13ca8 to 97a8aeb Aug 26, 2020
@jay
Copy link
Member

@jay jay commented Aug 27, 2020

I don't think there is a good reason to still support it, but can we just drop support for it without deprecating it first?

Yes. Who is using it? Unless you have Windows 95 that for some reason doesn't have winsock 2. And anyway who is building for that and how? I don't even know if that's still possible, this aside.

@mback2k
Copy link
Member Author

@mback2k mback2k commented Aug 27, 2020

Okay, fine with me. But does that then mean that we shouldn't check for WinSock version 2 and just assume USE_WINSOCK means version 2 and fail hard (due to functions not being available, etc.) if that is not the case? We already have these:

curl/lib/setup-win32.h

Lines 61 to 75 in fbe07c6

/*
* Define USE_WINSOCK to 2 if we have and use WINSOCK2 API, else
* define USE_WINSOCK to 1 if we have and use WINSOCK API, else
* undefine USE_WINSOCK.
*/
#undef USE_WINSOCK
#ifdef HAVE_WINSOCK2_H
# define USE_WINSOCK 2
#else
# ifdef HAVE_WINSOCK_H
# define USE_WINSOCK 1
# endif
#endif

curl/lib/system_win32.c

Lines 58 to 60 in fbe07c6

#if defined(ENABLE_IPV6) && (USE_WINSOCK < 2)
#error IPV6_requires_winsock2
#endif

curl/lib/telnet.c

Lines 201 to 239 in fbe07c6

#ifdef USE_WINSOCK
static CURLcode
check_wsock2(struct Curl_easy *data)
{
int err;
WORD wVersionRequested;
WSADATA wsaData;
DEBUGASSERT(data);
/* telnet requires at least WinSock 2.0 so ask for it. */
wVersionRequested = MAKEWORD(2, 0);
err = WSAStartup(wVersionRequested, &wsaData);
/* We must've called this once already, so this call */
/* should always succeed. But, just in case... */
if(err != 0) {
failf(data,"WSAStartup failed (%d)",err);
return CURLE_FAILED_INIT;
}
/* We have to have a WSACleanup call for every successful */
/* WSAStartup call. */
WSACleanup();
/* Check that our version is supported */
if(LOBYTE(wsaData.wVersion) != LOBYTE(wVersionRequested) ||
HIBYTE(wsaData.wVersion) != HIBYTE(wVersionRequested)) {
/* Our version isn't supported */
failf(data, "insufficient winsock version to support "
"telnet");
return CURLE_FAILED_INIT;
}
/* Our version is supported */
return CURLE_OK;
}
#endif

All of this could probably be simplified to compile time only checks if we just assume WinSock 2 and drop the dynamic library load, as suggested here: #5634 (comment)

@bagder
Copy link
Member

@bagder bagder commented Aug 27, 2020

does that then mean that we shouldn't check for WinSock version 2 and just assume USE_WINSOCK means version 2

Sure. Removing support for winsock1 that isn't used by anyone is also a benefit as it will make the code easier to read etc.

@mback2k
Copy link
Member Author

@mback2k mback2k commented Aug 27, 2020

does that then mean that we shouldn't check for WinSock version 2 and just assume USE_WINSOCK means version 2

Sure. Removing support for winsock1 that isn't used by anyone is also a benefit as it will make the code easier to read etc.

Okay, so I will start on reworking this PR into something to drop support for WinSock 1 and just assume WinSock 2 statically.

@mback2k mback2k marked this pull request as draft Aug 27, 2020
@bagder
Copy link
Member

@bagder bagder commented Aug 27, 2020

We should just document it as a requirement in docs/INTERNALS where we note other "at least this version" stuff.

@mback2k mback2k force-pushed the mback2k:multi-winsock2 branch 2 times, most recently from 18bb1fc to 40db989 Aug 28, 2020
@mback2k mback2k changed the title multi: socket readiness pre-check requires WinSock version 2 win32: drop support for WinSock version 1, require version 2 Aug 28, 2020
@mback2k mback2k force-pushed the mback2k:multi-winsock2 branch 2 times, most recently from a243719 to 42a00c9 Aug 28, 2020
lib/system_win32.c Outdated Show resolved Hide resolved
mback2k added 2 commits Aug 30, 2020
IPv6, telnet and now also the multi API require WinSock
version 2 which is available starting with Windows 95.

Therefore we think it is time to drop support for version 1.
Drop dynamic loading of ws2_32.dll and instead rely on the
imported version which is now required to be at least 2.2.
@mback2k mback2k force-pushed the mback2k:multi-winsock2 branch from 42a00c9 to c5bdbad Aug 30, 2020
@mback2k mback2k marked this pull request as ready for review Aug 30, 2020
@jay
jay approved these changes Aug 31, 2020
@mback2k
Copy link
Member Author

@mback2k mback2k commented Aug 31, 2020

@bagder what do you think? Are we ready to go and drop support for WinSock 1 or should we announce this first somehow?

@mback2k mback2k requested review from vszakats and removed request for bagder and vszakats Aug 31, 2020
@mback2k mback2k requested review from jay, MarcelRaad, bagder and vszakats Aug 31, 2020
@mback2k
Copy link
Member Author

@mback2k mback2k commented Aug 31, 2020

Stupid mobile app replaced existing reviewers instead of just adding new ones, sorry for that.

@bagder
bagder approved these changes Aug 31, 2020
lib/telnet.c Show resolved Hide resolved
@mback2k
Copy link
Member Author

@mback2k mback2k commented Sep 2, 2020

Thank you everyone, merging this now...

@mback2k mback2k closed this in 3e4b32a Sep 2, 2020
mback2k added a commit that referenced this pull request Sep 2, 2020
Drop dynamic loading of ws2_32.dll and instead rely on the
imported version which is now required to be at least 2.2.

Reviewed-by: Marcel Raad
Reviewed-by: Jay Satiro
Reviewed-by: Daniel Stenberg
Reviewed-by: Viktor Szakats

Closes #5854
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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