[PATCH v3 0/3] MR5195: ws2_32: Implement TCP_KEEPALIVE, TCP_KEEPCNT and TCP_KEEPINTVL socket options
Application "ZusiDisplay" wants to set these socket options. setsockopt() currently returns an error on wine, which prevents the application from working correctly.
I'm not really sure if I'm using the IOCTL_AFD_WINE_SET/GET_* correctly. Maybe the new ones should have different "function" numbers and be sorted below the one for TCP_NODELAY?
Also, not sure if anything needs to be done in unix/socket.c for portability. Are these TCP_KEEPIDLE etc. constants defined on all OSes supported by wine? If not, how should I handle this?
ZusiDisplay seems to work fine if the setsockopt() call is ignored in wine, without returning an error. So if this merge request is not that great, I'd be happy to send a merge request to ignore the call instead.
-- v3: ws2_32: Implement TCP_KEEP{ALIVE,CNT,INTVL} options. ws2_32/tests: Test TCP_KEEP{ALIVE,CNT,INTVL} options. include: Add TCP_KEEPCNT and TCP_KEEPINTVL definitions.
From: Florian Will florian.will@gmail.com
--- include/ws2ipdef.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/include/ws2ipdef.h b/include/ws2ipdef.h index fcb1f56c005..72e2dad2fa5 100644 --- a/include/ws2ipdef.h +++ b/include/ws2ipdef.h @@ -313,6 +313,8 @@ typedef struct WS(in6_pktinfo) { #define TCP_OFFLOAD_PREFERENCE 11 #define TCP_CONGESTION_ALGORITHM 12 #define TCP_DELAY_FIN_ACK 13 +#define TCP_KEEPCNT 16 +#define TCP_KEEPINTVL 17 #else /* WS_TCP_NODELAY is defined elsewhere */ #define WS_TCP_EXPEDITED_1122 2 @@ -327,6 +329,8 @@ typedef struct WS(in6_pktinfo) { #define WS_TCP_OFFLOAD_PREFERENCE 11 #define WS_TCP_CONGESTION_ALGORITHM 12 #define WS_TCP_DELAY_FIN_ACK 13 +#define WS_TCP_KEEPCNT 16 +#define WS_TCP_KEEPINTVL 17 #endif /* USE_WS_PREFIX */
#define PROTECTION_LEVEL_UNRESTRICTED 10
From: Florian Will florian.will@gmail.com
--- dlls/ws2_32/tests/sock.c | 44 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-)
diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index b0287fdd004..01c14d73de6 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -1193,6 +1193,7 @@ static void test_set_getsockopt(void) DWORD values[3]; BOOL accepts_large_value; BOOL bool_value; + BOOL allow_noprotoopt; /* for old windows or wine todo */ } test_optsize[] = { @@ -1210,6 +1211,9 @@ static void test_set_getsockopt(void) {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_SNDTIMEO, FALSE, {1, 2, 4}, {0}, TRUE}, {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_OPENTYPE, FALSE, {1, 2, 4}, {0}, TRUE}, {AF_INET, SOCK_STREAM, IPPROTO_TCP, TCP_NODELAY, TRUE, {1, 1, 1}, {0}, TRUE}, + {AF_INET, SOCK_STREAM, IPPROTO_TCP, TCP_KEEPALIVE, FALSE, {0, 0, 4}, {0}, TRUE, FALSE, TRUE}, /* wine todo */ + {AF_INET, SOCK_STREAM, IPPROTO_TCP, TCP_KEEPCNT, FALSE, {0, 0, 4}, {0}, FALSE, FALSE, TRUE}, /* win10+, wine todo*/ + {AF_INET, SOCK_STREAM, IPPROTO_TCP, TCP_KEEPINTVL, FALSE, {0, 0, 4}, {0}, TRUE, FALSE, TRUE}, /* win10+, wine todo */ {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_MULTICAST_LOOP, TRUE, {1, 1, 4}, {0}, TRUE, TRUE}, {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_MULTICAST_TTL, TRUE, {1, 1, 4}, {0}, FALSE}, {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_PKTINFO, FALSE, {0, 0, 4}, {0}, TRUE, TRUE}, @@ -1453,6 +1457,36 @@ static void test_set_getsockopt(void) ok(!err, "getsockopt TCP_NODELAY failed\n"); ok(!value, "TCP_NODELAY should be 0\n");
+ size = sizeof(DWORD); + value = 3600; + err = setsockopt(s, IPPROTO_TCP, TCP_KEEPALIVE, (char*)&value, 4); + todo_wine ok(!err, "setsockopt TCP_KEEPALIVE failed\n"); + value = 0; + err = getsockopt(s, IPPROTO_TCP, TCP_KEEPALIVE, (char*)&value, &size); + todo_wine ok(!err, "getsockopt TCP_KEEPALIVE failed\n"); + todo_wine ok(value == 3600, "TCP_KEEPALIVE should be 3600, is %ld\n", value); + + /* TCP_KEEPCNT and TCP_KEEPINTVL are supported on win10 and later */ + value = 5; + err = setsockopt(s, IPPROTO_TCP, TCP_KEEPCNT, (char*)&value, 4); + todo_wine ok(!err || broken(WSAGetLastError() == WSAENOPROTOOPT), + "setsockopt TCP_KEEPCNT failed: %d\n", WSAGetLastError()); + + if (!err) + { + value = 0; + err = getsockopt(s, IPPROTO_TCP, TCP_KEEPCNT, (char*)&value, &size); + ok(!err, "getsockopt TCP_KEEPCNT failed\n"); + ok(value == 5, "TCP_KEEPCNT should be 5, is %ld\n", value); + + err = setsockopt(s, IPPROTO_TCP, TCP_KEEPINTVL, (char*)&value, 4); + ok(!err, "setsockopt TCP_KEEPINTVL failed\n"); + value = 0; + err = getsockopt(s, IPPROTO_TCP, TCP_KEEPINTVL, (char*)&value, &size); + ok(!err, "getsockopt TCP_KEEPINTVL failed\n"); + ok(value == 5, "TCP_KEEPINTVL should be 5, is %ld\n", value); + } + /* Test for erroneously passing a value instead of a pointer as optval */ size = sizeof(char); err = setsockopt(s, SOL_SOCKET, SO_DONTROUTE, (char *)1, size); @@ -1517,7 +1551,15 @@ static void test_set_getsockopt(void)
size = sizeof(save_value); err = getsockopt(s2, test_optsize[i].level, test_optsize[i].optname, (char*)&save_value, &size); - ok(!err, "Unexpected getsockopt result %d.\n", err); + ok(!err || (test_optsize[i].allow_noprotoopt && WSAGetLastError() == WSAENOPROTOOPT), + "Unexpected getsockopt result %d.\n", err); + + if (err) + { + closesocket(s2); + winetest_pop_context(); + continue; + }
value64 = 0xffffffff00000001; err = setsockopt(s2, test_optsize[i].level, test_optsize[i].optname, (char *)&value64, sizeof(value64));
From: Florian Will florian.will@gmail.com
--- dlls/ntdll/unix/socket.c | 28 ++++++++++++++++++ dlls/ws2_32/socket.c | 63 ++++++++++++++++++++++++++++++++++++++++ dlls/ws2_32/tests/sock.c | 18 ++++++------ include/wine/afd.h | 6 ++++ 4 files changed, 106 insertions(+), 9 deletions(-)
diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index 4e6781df607..4b0dca37e7d 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -2507,6 +2507,34 @@ NTSTATUS sock_ioctl( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc case IOCTL_AFD_WINE_SET_TCP_NODELAY: return do_setsockopt( handle, io, IPPROTO_TCP, TCP_NODELAY, in_buffer, in_size );
+#if defined(TCP_KEEPIDLE) + /* TCP_KEEPALIVE on Windows is often called TCP_KEEPIDLE on Unix */ + case IOCTL_AFD_WINE_GET_TCP_KEEPALIVE: + return do_getsockopt( handle, io, IPPROTO_TCP, TCP_KEEPIDLE, out_buffer, out_size ); + + case IOCTL_AFD_WINE_SET_TCP_KEEPALIVE: + return do_setsockopt( handle, io, IPPROTO_TCP, TCP_KEEPIDLE, in_buffer, in_size ); +#elif defined(TCP_KEEPALIVE) + /* Mac */ + case IOCTL_AFD_WINE_GET_TCP_KEEPALIVE: + return do_getsockopt( handle, io, IPPROTO_TCP, TCP_KEEPALIVE, out_buffer, out_size ); + + case IOCTL_AFD_WINE_SET_TCP_KEEPALIVE: + return do_setsockopt( handle, io, IPPROTO_TCP, TCP_KEEPALIVE, in_buffer, in_size ); +#endif + + case IOCTL_AFD_WINE_GET_TCP_KEEPINTVL: + return do_getsockopt( handle, io, IPPROTO_TCP, TCP_KEEPINTVL, out_buffer, out_size ); + + case IOCTL_AFD_WINE_SET_TCP_KEEPINTVL: + return do_setsockopt( handle, io, IPPROTO_TCP, TCP_KEEPINTVL, in_buffer, in_size ); + + case IOCTL_AFD_WINE_GET_TCP_KEEPCNT: + return do_getsockopt( handle, io, IPPROTO_TCP, TCP_KEEPCNT, out_buffer, out_size ); + + case IOCTL_AFD_WINE_SET_TCP_KEEPCNT: + return do_setsockopt( handle, io, IPPROTO_TCP, TCP_KEEPCNT, in_buffer, in_size ); + default: { if ((code >> 16) == FILE_DEVICE_NETWORK) diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index 6aab249a1b8..eb84558cbac 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -1931,6 +1931,36 @@ int WINAPI getsockopt( SOCKET s, int level, int optname, char *optval, int *optl *optlen = 1; return server_getsockopt( s, IOCTL_AFD_WINE_GET_TCP_NODELAY, optval, optlen );
+ case TCP_KEEPALIVE: + if (*optlen < sizeof(DWORD) || !optval) + { + *optlen = 0; + SetLastError( WSAEFAULT ); + return SOCKET_ERROR; + } + *optlen = sizeof(DWORD); + return server_getsockopt( s, IOCTL_AFD_WINE_GET_TCP_KEEPALIVE, optval, optlen ); + + case TCP_KEEPCNT: + if (*optlen < sizeof(DWORD) || !optval) + { + *optlen = 0; + SetLastError( WSAEFAULT ); + return SOCKET_ERROR; + } + *optlen = sizeof(DWORD); + return server_getsockopt( s, IOCTL_AFD_WINE_GET_TCP_KEEPCNT, optval, optlen ); + + case TCP_KEEPINTVL: + if (*optlen < sizeof(DWORD) || !optval) + { + *optlen = 0; + SetLastError( WSAEFAULT ); + return SOCKET_ERROR; + } + *optlen = sizeof(DWORD); + return server_getsockopt( s, IOCTL_AFD_WINE_GET_TCP_KEEPINTVL, optval, optlen ); + default: FIXME( "unrecognized TCP option %#x\n", optname ); SetLastError( WSAENOPROTOOPT ); @@ -3325,6 +3355,12 @@ int WINAPI setsockopt( SOCKET s, int level, int optname, const char *optval, int break; /* case NSPROTO_IPX */
case IPPROTO_TCP: + if (optlen < 0) + { + SetLastError(WSAENOBUFS); + return SOCKET_ERROR; + } + switch(optname) { case TCP_NODELAY: @@ -3336,6 +3372,33 @@ int WINAPI setsockopt( SOCKET s, int level, int optname, const char *optval, int value = *optval; return server_setsockopt( s, IOCTL_AFD_WINE_SET_TCP_NODELAY, (char*)&value, sizeof(value) );
+ case TCP_KEEPALIVE: + if (optlen < sizeof(DWORD) || !optval) + { + SetLastError( WSAEFAULT ); + return SOCKET_ERROR; + } + value = *(DWORD*)optval; + return server_setsockopt( s, IOCTL_AFD_WINE_SET_TCP_KEEPALIVE, (char*)&value, sizeof(value) ); + + case TCP_KEEPCNT: + if (optlen < sizeof(DWORD) || !optval) + { + SetLastError( WSAEFAULT ); + return SOCKET_ERROR; + } + value = *(DWORD*)optval; + return server_setsockopt( s, IOCTL_AFD_WINE_SET_TCP_KEEPCNT, (char*)&value, sizeof(value) ); + + case TCP_KEEPINTVL: + if (optlen < sizeof(DWORD) || !optval) + { + SetLastError( WSAEFAULT ); + return SOCKET_ERROR; + } + value = *(DWORD*)optval; + return server_setsockopt( s, IOCTL_AFD_WINE_SET_TCP_KEEPINTVL, (char*)&value, sizeof(value) ); + default: FIXME("Unknown IPPROTO_TCP optname 0x%08x\n", optname); SetLastError(WSAENOPROTOOPT); diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 01c14d73de6..6954b3e077a 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -1193,7 +1193,7 @@ static void test_set_getsockopt(void) DWORD values[3]; BOOL accepts_large_value; BOOL bool_value; - BOOL allow_noprotoopt; /* for old windows or wine todo */ + BOOL allow_noprotoopt; /* for old windows only, must work on wine */ } test_optsize[] = { @@ -1211,9 +1211,9 @@ static void test_set_getsockopt(void) {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_SNDTIMEO, FALSE, {1, 2, 4}, {0}, TRUE}, {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_OPENTYPE, FALSE, {1, 2, 4}, {0}, TRUE}, {AF_INET, SOCK_STREAM, IPPROTO_TCP, TCP_NODELAY, TRUE, {1, 1, 1}, {0}, TRUE}, - {AF_INET, SOCK_STREAM, IPPROTO_TCP, TCP_KEEPALIVE, FALSE, {0, 0, 4}, {0}, TRUE, FALSE, TRUE}, /* wine todo */ - {AF_INET, SOCK_STREAM, IPPROTO_TCP, TCP_KEEPCNT, FALSE, {0, 0, 4}, {0}, FALSE, FALSE, TRUE}, /* win10+, wine todo*/ - {AF_INET, SOCK_STREAM, IPPROTO_TCP, TCP_KEEPINTVL, FALSE, {0, 0, 4}, {0}, TRUE, FALSE, TRUE}, /* win10+, wine todo */ + {AF_INET, SOCK_STREAM, IPPROTO_TCP, TCP_KEEPALIVE, FALSE, {0, 0, 4}, {0}, TRUE}, + {AF_INET, SOCK_STREAM, IPPROTO_TCP, TCP_KEEPCNT, FALSE, {0, 0, 4}, {0}, FALSE, FALSE, TRUE}, /* win10+ */ + {AF_INET, SOCK_STREAM, IPPROTO_TCP, TCP_KEEPINTVL, FALSE, {0, 0, 4}, {0}, TRUE, FALSE, TRUE}, /* win10+ */ {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_MULTICAST_LOOP, TRUE, {1, 1, 4}, {0}, TRUE, TRUE}, {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_MULTICAST_TTL, TRUE, {1, 1, 4}, {0}, FALSE}, {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_PKTINFO, FALSE, {0, 0, 4}, {0}, TRUE, TRUE}, @@ -1460,16 +1460,16 @@ static void test_set_getsockopt(void) size = sizeof(DWORD); value = 3600; err = setsockopt(s, IPPROTO_TCP, TCP_KEEPALIVE, (char*)&value, 4); - todo_wine ok(!err, "setsockopt TCP_KEEPALIVE failed\n"); + ok(!err, "setsockopt TCP_KEEPALIVE failed\n"); value = 0; err = getsockopt(s, IPPROTO_TCP, TCP_KEEPALIVE, (char*)&value, &size); - todo_wine ok(!err, "getsockopt TCP_KEEPALIVE failed\n"); - todo_wine ok(value == 3600, "TCP_KEEPALIVE should be 3600, is %ld\n", value); + ok(!err, "getsockopt TCP_KEEPALIVE failed\n"); + ok(value == 3600, "TCP_KEEPALIVE should be 3600, is %ld\n", value);
/* TCP_KEEPCNT and TCP_KEEPINTVL are supported on win10 and later */ value = 5; err = setsockopt(s, IPPROTO_TCP, TCP_KEEPCNT, (char*)&value, 4); - todo_wine ok(!err || broken(WSAGetLastError() == WSAENOPROTOOPT), + ok(!err || broken(WSAGetLastError() == WSAENOPROTOOPT), "setsockopt TCP_KEEPCNT failed: %d\n", WSAGetLastError());
if (!err) @@ -1551,7 +1551,7 @@ static void test_set_getsockopt(void)
size = sizeof(save_value); err = getsockopt(s2, test_optsize[i].level, test_optsize[i].optname, (char*)&save_value, &size); - ok(!err || (test_optsize[i].allow_noprotoopt && WSAGetLastError() == WSAENOPROTOOPT), + ok(!err || broken(test_optsize[i].allow_noprotoopt && WSAGetLastError() == WSAENOPROTOOPT), "Unexpected getsockopt result %d.\n", err);
if (err) diff --git a/include/wine/afd.h b/include/wine/afd.h index 788adb4a495..aba559ebd8a 100644 --- a/include/wine/afd.h +++ b/include/wine/afd.h @@ -285,6 +285,12 @@ C_ASSERT( sizeof(struct afd_get_events_params) == 56 ); #define IOCTL_AFD_WINE_SET_IP_RECVTOS WINE_AFD_IOC(296) #define IOCTL_AFD_WINE_GET_SO_EXCLUSIVEADDRUSE WINE_AFD_IOC(297) #define IOCTL_AFD_WINE_SET_SO_EXCLUSIVEADDRUSE WINE_AFD_IOC(298) +#define IOCTL_AFD_WINE_GET_TCP_KEEPALIVE WINE_AFD_IOC(299) +#define IOCTL_AFD_WINE_SET_TCP_KEEPALIVE WINE_AFD_IOC(300) +#define IOCTL_AFD_WINE_GET_TCP_KEEPCNT WINE_AFD_IOC(301) +#define IOCTL_AFD_WINE_SET_TCP_KEEPCNT WINE_AFD_IOC(302) +#define IOCTL_AFD_WINE_GET_TCP_KEEPINTVL WINE_AFD_IOC(303) +#define IOCTL_AFD_WINE_SET_TCP_KEEPINTVL WINE_AFD_IOC(304)
struct afd_iovec {
Hi,
It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated.
The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details:
The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=143600
Your paranoid android.
=== w1064_2qxl (64 bit report) ===
ws2_32: sock.c:6294: Test failed: failed to wait for thread termination: 0
On Fri Mar 1 09:00:50 2024 +0000, Zebediah Figura wrote:
Also, not sure if anything needs to be done in unix/socket.c for
portability. Are these TCP_KEEPIDLE etc. constants defined on all OSes supported by wine? If not, how should I handle this? A brief search suggests that Solaris, FreeBSD, and NetBSD all have them. Mac has them as well, but it calls it TCP_KEEPALIVE like Windows, not TCP_KEEPIDLE, so we should check for that. If we do break compilation anywhere, I suspect it's fine to wait for a report and fix it after the fact.
I added some #ifs to hopefully make it work on Mac. I have no way to test it though, and the Mac CI runner is currently offline. So let's see. :-)
On Fri Mar 1 09:00:51 2024 +0000, Zebediah Figura wrote:
We've had problems in the past with applications being very picky about how we do parameter validation, so I'd like to be proactive about getting that right, or at least not copy it from elsewhere without testing. The relevant tests are a bit tricky to read, so I've taken the liberty of adding them and adjusting the implementation appropriately; would you mind fixing up your patches accordingly? [scratch.diff](/uploads/ecf33271bf2f18bce38f7ef6daf40aa7/scratch.diff)
Thanks a lot, that's pretty useful! I had to add a new member to the test definition struct for old windows (and initially the wine todo), I hope that's okay. If you'd prefer a different approach, just let me know.
On Fri Mar 1 09:00:51 2024 +0000, Zebediah Figura wrote:
+ /* TCP_KEEPCNT and TCP_KEEPINTVL are supported on win10 and later */ + value = 5; + err = setsockopt(s, IPPROTO_TCP, TCP_KEEPCNT, (char*)&value, 4); + if (!err || WSAGetLastError() != WSAENOPROTOOPT || winetest_platform_is_wine) + { + todo_wine ok(!err, "setsockopt TCP_KEEPCNT failed: %d\n", WSAGetLastError()); + value = 0; + err = getsockopt(s, IPPROTO_TCP, TCP_KEEPCNT, (char*)&value, &size); + todo_wine ok(!err, "getsockopt TCP_KEEPCNT failed\n"); + todo_wine ok(value == 5, "TCP_KEEPCNT should be 5, is %ld\n", value);I'd rather write this as:
+ err = setsockopt(s, IPPROTO_TCP, TCP_KEEPCNT, (char*)&value, 4); + todo_wine ok(!err || broken(WSAGetLastError() != WSAENOPROTOOPT), "..."); + if (!err) + { + value = 0; + err = getsockopt(s, IPPROTO_TCP, TCP_KEEPCNT, (char*)&value, &size); + todo_wine ok(!err, "getsockopt TCP_KEEPCNT failed\n"); + todo_wine ok(value == 5, "TCP_KEEPCNT should be 5, is %ld\n", value);
Thanks, I changed it according to your suggestion. I used `== WSAENOPROTOOPT` instead of `!= WSAENOPROTOOPT` as that seems to be correct here.
On Fri Mar 1 09:00:50 2024 +0000, Florian Will wrote:
I added some #ifs to hopefully make it work on Mac. I have no way to test it though, and the Mac CI runner is currently offline. So let's see. :-)
The Mac CI runner came back online just at the right time apparently, looks like the build succeeded.
On Fri Mar 1 09:00:51 2024 +0000, Florian Will wrote:
Thanks, I changed it according to your suggestion. I used `== WSAENOPROTOOPT` instead of `!= WSAENOPROTOOPT` as that seems to be correct here.
Oops, yes, that was a typographical error on my end.
This merge request was approved by Zebediah Figura.
-
Florian Will -
Florian Will (＠w-flo)
-
Marvin -
Zebediah Figura (＠zfigura)