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

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

mback2k
Copy link
Member

@mback2k mback2k commented Aug 25, 2020 •
edited

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 August 25, 2020 20:01
@mback2k mback2k self-assigned this Aug 25, 2020
@mback2k mback2k added the Windows Windows-specific label Aug 25, 2020
@jay
Copy link
Member

jay commented Aug 26, 2020

Is there really a reason to continue supporting Winsock 1

@mback2k
Copy link
Member Author

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 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.

@jay
Copy link
Member

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 commented Aug 27, 2020 •
edited

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 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 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 August 27, 2020 09:02
@bagder
Copy link
Member

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 multi-winsock2 branch 2 times, most recently from 18bb1fc to 40db989 Compare August 28, 2020 19:13
@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 multi-winsock2 branch 2 times, most recently from a243719 to 42a00c9 Compare August 28, 2020 19:54
MarcelRaad
MarcelRaad reviewed Aug 28, 2020
View reviewed changes
lib/system_win32.c Outdated Show resolved Hide resolved
@mback2k
win32: drop support for WinSock version 1, require version 2
09d5bb9 
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.
@mback2k
telnet.c: depend on static requirement of WinSock version 2
c5bdbad 
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 marked this pull request as ready for review August 30, 2020 15:43
@mback2k
Copy link
Member Author

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 August 31, 2020 11:00
@mback2k
Copy link
Member Author

mback2k commented Aug 31, 2020

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

vszakats
vszakats reviewed Aug 31, 2020
View reviewed changes
lib/telnet.c Show resolved Hide resolved
@mback2k
Copy link
Member Author

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
@mback2k
telnet.c: depend on static requirement of WinSock version 2
8bdc4f8 
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
Reviewers

@bagder bagder bagder approved these changes

@vszakats vszakats vszakats approved these changes

@MarcelRaad MarcelRaad MarcelRaad approved these changes

@jay jay Awaiting requested review from jay

Assignees

@mback2k mback2k

Labels
Windows Windows-specific
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mback2k @jay @bagder @vszakats @MarcelRaad