[PATCH v2 1/6] ws2_32/tests: Add tests for setting and getting socket options with shorter length.
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- v2: - also check for nonzero optval where appropriate throughout the patchset. The aim of the patchset is to fix setting and getting 32bit sized socket options when the beaviour is different between Windows and Linux setsockopt() and getsockopt(). That behaviour partially regressed on the move from ws2_32 to ntdll: setsockopt() had the code which would adjust the size for option with the provided length less than 4 bytes (which helped, e. g., SO_REUSEADDR and SO_KEEPALIVE and Killing Floor 2 depended on that). ALthough, as my tests show, that was not universally correct across all the options and the details of getsockopt() were also different. I have another 16 patches on top of this series addressing the other options. There are also a few options (e. g., SO_KEEPALIVE) which should use the first byte only (like that is currently done for TCP_NODELAY minus accessing optval before NULL check). Also a few options behave like boolean values (return value of 1 after setting 0x100. But since fixing up those are orthogonal to what these patches are doing (i. e., the present patches do not change the behaviour for currently supported 4 byte optlen) I am fixing up those in the yet further patches. dlls/ws2_32/tests/sock.c | 86 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 85 insertions(+), 1 deletion(-) diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 6b6267dc153..0eece82bfb1 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -1155,8 +1155,27 @@ static const LINGER linger_testvals[] = { static void test_set_getsockopt(void) { + static struct + { + int af; + int type; + int level; + int optname; + BOOL accepts_short_len; + unsigned int sizes[3]; + DWORD values[3]; + } + test_optsize[] = + { + {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_RCVTIMEO, FALSE, {1, 2, 4}}, + {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_SNDTIMEO, FALSE, {1, 2, 4}}, + {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_MULTICAST_LOOP, TRUE, {1, 1, 4}}, + {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_MULTICAST_TTL, TRUE, {1, 1, 4}}, + {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_TOS, TRUE, {1, 1, 4}}, + {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_TTL, TRUE, {1, 1, 4}}, + }; SOCKET s, s2; - int i, err, lasterr; + int i, j, err, lasterr; int timeout; LINGER lingval; int size; @@ -1375,6 +1394,71 @@ static void test_set_getsockopt(void) err, WSAGetLastError()); closesocket(s); + + /* Test option short length. */ + for (i = 0; i < ARRAY_SIZE(test_optsize); ++i) + { + s2 = socket( test_optsize[i].af, test_optsize[i].type, 0 ); + ok(s2 != INVALID_SOCKET, "socket() failed error %d\n", WSAGetLastError()); + for (j = 0; j < 3; ++j) + { + DWORD expected_last_error, expected_value, expected_size, save_value; + int expected_err; + + winetest_push_context("i %u, level %d, optname %d, len %u", + i, test_optsize[i].level, test_optsize[i].optname, 1 << j); + + 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); + + if (test_optsize[i].accepts_short_len || j == 2) + { + expected_err = 0; + expected_last_error = ERROR_SUCCESS; + expected_size = test_optsize[i].sizes[j] ? test_optsize[i].sizes[j] : 4; + if (test_optsize[i].values[j]) + expected_value = test_optsize[i].values[j]; + else if (expected_size == 4) + expected_value = 1; + else + expected_value = (0xdeadbeef & ~((1 << expected_size * 8) - 1)) | 1; + } + else + { + expected_err = -1; + expected_last_error = WSAEFAULT; + expected_size = test_optsize[i].sizes[j]; + if (test_optsize[i].values[j]) + expected_value = test_optsize[i].values[j]; + else + expected_value = 0xdeadbeef; + } + + value = 1; + SetLastError(0xdeadbeef); + err = setsockopt(s2, test_optsize[i].level, test_optsize[i].optname, (char*)&value, 1 << j); + ok(err == expected_err, "Unexpected setsockopt result %d.\n", err); + ok(WSAGetLastError() == expected_last_error, "Unexpected WSAGetLastError() %u.\n", WSAGetLastError()); + + value = 0xdeadbeef; + SetLastError(0xdeadbeef); + size = 1 << j; + err = getsockopt(s2, test_optsize[i].level, test_optsize[i].optname, (char*)&value, &size); + ok(err == expected_err, "Unexpected getsockopt result %d.\n", err); + ok(WSAGetLastError() == expected_last_error, "Unexpected WSAGetLastError() %u.\n", WSAGetLastError()); + ok(value == expected_value, "Got unexpected value %#x, expected %#x.\n", value, expected_value); + ok(size == expected_size, "Got unexpected size %u, expected %u.\n", size, expected_size); + + err = setsockopt(s2, test_optsize[i].level, test_optsize[i].optname, + (char*)&save_value, sizeof(save_value)); + ok(!err, "Unexpected getsockopt result %d.\n", err); + + winetest_pop_context(); + } + closesocket(s2); + } + /* Test with the closed socket */ SetLastError(0xdeadbeef); size = sizeof(i); -- 2.35.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/socket.c | 15 ++++++++++++--- dlls/ws2_32/tests/sock.c | 1 + 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index e5e29107853..283404baf5a 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -1412,7 +1412,9 @@ int WINAPI getsockopt( SOCKET s, int level, int optname, char *optval, int *optl return server_getsockopt( s, IOCTL_AFD_WINE_GET_SO_ACCEPTCONN, optval, optlen ); case SO_BROADCAST: - return server_getsockopt( s, IOCTL_AFD_WINE_GET_SO_BROADCAST, optval, optlen ); + ret = server_getsockopt( s, IOCTL_AFD_WINE_GET_SO_BROADCAST, optval, optlen ); + if (!ret && *optlen < sizeof(DWORD)) *optlen = 1; + return ret; case SO_BSP_STATE: { @@ -2769,6 +2771,8 @@ static int server_setsockopt( SOCKET s, ULONG code, const char *optval, int optl */ int WINAPI setsockopt( SOCKET s, int level, int optname, const char *optval, int optlen ) { + DWORD value = 0; + TRACE( "socket %#Ix, %s, optval %s, optlen %d\n", s, debugstr_sockopt(level, optname), debugstr_optval(optval, optlen), optlen ); @@ -2785,8 +2789,13 @@ int WINAPI setsockopt( SOCKET s, int level, int optname, const char *optval, int switch(optname) { case SO_BROADCAST: - return server_setsockopt( s, IOCTL_AFD_WINE_SET_SO_BROADCAST, optval, optlen ); - + if (optlen <= 0 || !optval) + { + SetLastError(WSAEFAULT); + return SOCKET_ERROR; + } + memcpy( &value, optval, min( optlen, sizeof(value) )); + return server_setsockopt( s, IOCTL_AFD_WINE_SET_SO_BROADCAST, (char *)&value, sizeof(value) ); case SO_DONTLINGER: { struct linger linger; diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 0eece82bfb1..56ef78ad040 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -1167,6 +1167,7 @@ static void test_set_getsockopt(void) } test_optsize[] = { + {AF_INET, SOCK_DGRAM, SOL_SOCKET, SO_BROADCAST, TRUE, {1, 1, 4}, {0, 0xdead0001, 0}}, {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_RCVTIMEO, FALSE, {1, 2, 4}}, {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_SNDTIMEO, FALSE, {1, 2, 4}}, {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_MULTICAST_LOOP, TRUE, {1, 1, 4}}, -- 2.35.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/socket.c | 12 +++++++----- dlls/ws2_32/tests/sock.c | 1 + 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index 283404baf5a..b7da3649806 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -1481,11 +1481,12 @@ int WINAPI getsockopt( SOCKET s, int level, int optname, char *optval, int *optl case SO_DONTLINGER: { - struct linger linger; + LINGER linger; int len = sizeof(linger); + BOOL value; int ret; - if (!optlen || *optlen < sizeof(BOOL)|| !optval) + if (!optlen || *optlen < 1 || !optval) { SetLastError(WSAEFAULT); return SOCKET_ERROR; @@ -1493,8 +1494,9 @@ int WINAPI getsockopt( SOCKET s, int level, int optname, char *optval, int *optl if (!(ret = getsockopt( s, SOL_SOCKET, SO_LINGER, (char *)&linger, &len ))) { - *(BOOL *)optval = !linger.l_onoff; - *optlen = sizeof(BOOL); + value = !linger.l_onoff; + memcpy( optval, &value, min( sizeof(BOOL), *optlen )); + *optlen = *optlen >= sizeof(BOOL) ? sizeof(BOOL) : 1; } return ret; } @@ -2798,7 +2800,7 @@ int WINAPI setsockopt( SOCKET s, int level, int optname, const char *optval, int return server_setsockopt( s, IOCTL_AFD_WINE_SET_SO_BROADCAST, (char *)&value, sizeof(value) ); case SO_DONTLINGER: { - struct linger linger; + LINGER linger; if (!optval) { diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 56ef78ad040..5d8c1b169e1 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -1168,6 +1168,7 @@ static void test_set_getsockopt(void) test_optsize[] = { {AF_INET, SOCK_DGRAM, SOL_SOCKET, SO_BROADCAST, TRUE, {1, 1, 4}, {0, 0xdead0001, 0}}, + {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_DONTLINGER, TRUE, {1, 1, 4}, {0, 0xdead0001, 0}}, {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_RCVTIMEO, FALSE, {1, 2, 4}}, {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_SNDTIMEO, FALSE, {1, 2, 4}}, {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_MULTICAST_LOOP, TRUE, {1, 1, 4}}, -- 2.35.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/socket.c | 8 +++++--- dlls/ws2_32/tests/sock.c | 1 + 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index b7da3649806..3ab9cd6a869 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -1504,13 +1504,14 @@ int WINAPI getsockopt( SOCKET s, int level, int optname, char *optval, int *optl /* As mentioned in setsockopt, Windows ignores this, so we * always return true here */ case SO_DONTROUTE: - if (!optlen || *optlen < sizeof(BOOL) || !optval) + if (!optlen || *optlen < 1 || !optval) { SetLastError(WSAEFAULT); return SOCKET_ERROR; } - *(BOOL *)optval = TRUE; - *optlen = sizeof(BOOL); + *optval = TRUE; + *optlen = 1; + SetLastError( ERROR_SUCCESS ); return 0; case SO_ERROR: @@ -2851,6 +2852,7 @@ int WINAPI setsockopt( SOCKET s, int level, int optname, const char *optval, int * socket. According to MSDN, this option is silently ignored.*/ case SO_DONTROUTE: TRACE("Ignoring SO_DONTROUTE\n"); + SetLastError( ERROR_SUCCESS ); return 0; /* Stops two sockets from being bound to the same port. Always happens diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 5d8c1b169e1..bfc688a8422 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -1169,6 +1169,7 @@ static void test_set_getsockopt(void) { {AF_INET, SOCK_DGRAM, SOL_SOCKET, SO_BROADCAST, TRUE, {1, 1, 4}, {0, 0xdead0001, 0}}, {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_DONTLINGER, TRUE, {1, 1, 4}, {0, 0xdead0001, 0}}, + {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_DONTROUTE, TRUE, {1, 1, 1}}, {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_RCVTIMEO, FALSE, {1, 2, 4}}, {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_SNDTIMEO, FALSE, {1, 2, 4}}, {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_MULTICAST_LOOP, TRUE, {1, 1, 4}}, -- 2.35.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/socket.c | 14 +++++++++++++- dlls/ws2_32/tests/sock.c | 1 + 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index 3ab9cd6a869..f383f91dfa5 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -1518,6 +1518,12 @@ int WINAPI getsockopt( SOCKET s, int level, int optname, char *optval, int *optl return server_getsockopt( s, IOCTL_AFD_WINE_GET_SO_ERROR, optval, optlen ); case SO_KEEPALIVE: + if (!optlen || *optlen < 1 || !optval) + { + SetLastError(WSAEFAULT); + return SOCKET_ERROR; + } + *optlen = 1; return server_getsockopt( s, IOCTL_AFD_WINE_GET_SO_KEEPALIVE, optval, optlen ); case SO_LINGER: @@ -2820,7 +2826,13 @@ int WINAPI setsockopt( SOCKET s, int level, int optname, const char *optval, int return -1; case SO_KEEPALIVE: - return server_setsockopt( s, IOCTL_AFD_WINE_SET_SO_KEEPALIVE, optval, optlen ); + if (optlen <= 0 || !optval) + { + SetLastError(WSAEFAULT); + return SOCKET_ERROR; + } + memcpy( &value, optval, min( optlen, sizeof(value) )); + return server_setsockopt( s, IOCTL_AFD_WINE_SET_SO_KEEPALIVE, (char *)&value, sizeof(value) ); case SO_LINGER: return server_setsockopt( s, IOCTL_AFD_WINE_SET_SO_LINGER, optval, optlen ); diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index bfc688a8422..5850bf38f1b 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -1169,6 +1169,7 @@ static void test_set_getsockopt(void) { {AF_INET, SOCK_DGRAM, SOL_SOCKET, SO_BROADCAST, TRUE, {1, 1, 4}, {0, 0xdead0001, 0}}, {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_DONTLINGER, TRUE, {1, 1, 4}, {0, 0xdead0001, 0}}, + {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_KEEPALIVE, TRUE, {1, 1, 1}}, {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_DONTROUTE, TRUE, {1, 1, 1}}, {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_RCVTIMEO, FALSE, {1, 2, 4}}, {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_SNDTIMEO, FALSE, {1, 2, 4}}, -- 2.35.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/socket.c | 7 +++++++ dlls/ws2_32/tests/sock.c | 1 + 2 files changed, 8 insertions(+) diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index f383f91dfa5..703464505ce 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -1534,6 +1534,8 @@ int WINAPI getsockopt( SOCKET s, int level, int optname, char *optval, int *optl /* struct linger and LINGER have different sizes */ if (!optlen || *optlen < sizeof(LINGER) || !optval) { + if (optlen && *optlen > 0 && optval) + memset( optval, 0, *optlen ); SetLastError(WSAEFAULT); return SOCKET_ERROR; } @@ -2835,6 +2837,11 @@ int WINAPI setsockopt( SOCKET s, int level, int optname, const char *optval, int return server_setsockopt( s, IOCTL_AFD_WINE_SET_SO_KEEPALIVE, (char *)&value, sizeof(value) ); case SO_LINGER: + if (!optlen || optlen < sizeof(LINGER) || !optval) + { + SetLastError(WSAEFAULT); + return SOCKET_ERROR; + } return server_setsockopt( s, IOCTL_AFD_WINE_SET_SO_LINGER, optval, optlen ); case SO_OOBINLINE: diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 5850bf38f1b..9a3e1994f49 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -1171,6 +1171,7 @@ static void test_set_getsockopt(void) {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_DONTLINGER, TRUE, {1, 1, 4}, {0, 0xdead0001, 0}}, {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_KEEPALIVE, TRUE, {1, 1, 1}}, {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_DONTROUTE, TRUE, {1, 1, 1}}, + {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_LINGER, FALSE, {1, 2, 4}, {0xdeadbe00, 0xdead0000}}, {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_RCVTIMEO, FALSE, {1, 2, 4}}, {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_SNDTIMEO, FALSE, {1, 2, 4}}, {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_MULTICAST_LOOP, TRUE, {1, 1, 4}}, -- 2.35.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=109729 Your paranoid android. === w8 (32 bit report) === ws2_32: sock.c:6618: Test failed: wrong count 0 sock.c:6627: Test failed: wrong count 8
On 3/3/22 15:16, Paul Gofman wrote:
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- v2: - also check for nonzero optval where appropriate throughout the patchset.
The aim of the patchset is to fix setting and getting 32bit sized socket options when the beaviour is different between Windows and Linux setsockopt() and getsockopt(). That behaviour partially regressed on the move from ws2_32 to ntdll: setsockopt() had the code which would adjust the size for option with the provided length less than 4 bytes (which helped, e. g., SO_REUSEADDR and SO_KEEPALIVE and Killing Floor 2 depended on that). ALthough, as my tests show, that was not universally correct across all the options and the details of getsockopt() were also different. I have another 16 patches on top of this series addressing the other options.
There are also a few options (e. g., SO_KEEPALIVE) which should use the first byte only (like that is currently done for TCP_NODELAY minus accessing optval before NULL check). Also a few options behave like boolean values (return value of 1 after setting 0x100. But since fixing up those are orthogonal to what these patches are doing (i. e., the present patches do not change the behaviour for currently supported 4 byte optlen) I am fixing up those in the yet further patches.
dlls/ws2_32/tests/sock.c | 86 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 85 insertions(+), 1 deletion(-)
diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 6b6267dc153..0eece82bfb1 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -1155,8 +1155,27 @@ static const LINGER linger_testvals[] = {
static void test_set_getsockopt(void) { + static struct + { + int af; + int type; + int level; + int optname; + BOOL accepts_short_len; + unsigned int sizes[3]; + DWORD values[3]; + } + test_optsize[] = + { + {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_RCVTIMEO, FALSE, {1, 2, 4}}, + {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_SNDTIMEO, FALSE, {1, 2, 4}}, + {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_MULTICAST_LOOP, TRUE, {1, 1, 4}}, + {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_MULTICAST_TTL, TRUE, {1, 1, 4}}, + {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_TOS, TRUE, {1, 1, 4}}, + {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_TTL, TRUE, {1, 1, 4}}, + }; SOCKET s, s2; - int i, err, lasterr; + int i, j, err, lasterr; int timeout; LINGER lingval; int size; @@ -1375,6 +1394,71 @@ static void test_set_getsockopt(void) err, WSAGetLastError());
closesocket(s); + + /* Test option short length. */ + for (i = 0; i < ARRAY_SIZE(test_optsize); ++i) + { + s2 = socket( test_optsize[i].af, test_optsize[i].type, 0 ); + ok(s2 != INVALID_SOCKET, "socket() failed error %d\n", WSAGetLastError()); + for (j = 0; j < 3; ++j)
ARRAY_SIZE(test_optsize[i].sizes)? The way this test is structured does not make it very easy to read, but I'm struggling to find a better option. The individual sockopts seem very idiosyncratic... Finding a way to include TCP_NODELAY in this table, as well as finding a way to test lengths less than 1 and greater than 4, would probably be helpful. I don't know if you have those in your pending patches.
+ { + DWORD expected_last_error, expected_value, expected_size, save_value; + int expected_err; + + winetest_push_context("i %u, level %d, optname %d, len %u", + i, test_optsize[i].level, test_optsize[i].optname, 1 << j); + + 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); + + if (test_optsize[i].accepts_short_len || j == 2)
"j == 2" is not great, maybe "size = 1 << j;" earlier, and then "size == 4" here, would be clearer.
+ { + expected_err = 0; + expected_last_error = ERROR_SUCCESS; + expected_size = test_optsize[i].sizes[j] ? test_optsize[i].sizes[j] : 4; + if (test_optsize[i].values[j]) + expected_value = test_optsize[i].values[j]; + else if (expected_size == 4) + expected_value = 1; + else + expected_value = (0xdeadbeef & ~((1 << expected_size * 8) - 1)) | 1;
This is not exactly easy to read. Maybe something like the following would be clearer? value = 1; expected_value = 0xdeadbeef; memcpy(&expected_value, &value, expected_size);
+ } + else + { + expected_err = -1; + expected_last_error = WSAEFAULT; + expected_size = test_optsize[i].sizes[j]; + if (test_optsize[i].values[j]) + expected_value = test_optsize[i].values[j]; + else + expected_value = 0xdeadbeef; + } + + value = 1; + SetLastError(0xdeadbeef); + err = setsockopt(s2, test_optsize[i].level, test_optsize[i].optname, (char*)&value, 1 << j); + ok(err == expected_err, "Unexpected setsockopt result %d.\n", err); + ok(WSAGetLastError() == expected_last_error, "Unexpected WSAGetLastError() %u.\n", WSAGetLastError()); + + value = 0xdeadbeef; + SetLastError(0xdeadbeef); + size = 1 << j; + err = getsockopt(s2, test_optsize[i].level, test_optsize[i].optname, (char*)&value, &size); + ok(err == expected_err, "Unexpected getsockopt result %d.\n", err); + ok(WSAGetLastError() == expected_last_error, "Unexpected WSAGetLastError() %u.\n", WSAGetLastError()); + ok(value == expected_value, "Got unexpected value %#x, expected %#x.\n", value, expected_value); + ok(size == expected_size, "Got unexpected size %u, expected %u.\n", size, expected_size); + + err = setsockopt(s2, test_optsize[i].level, test_optsize[i].optname, + (char*)&save_value, sizeof(save_value)); + ok(!err, "Unexpected getsockopt result %d.\n", err); + + winetest_pop_context(); + } + closesocket(s2); + } + /* Test with the closed socket */ SetLastError(0xdeadbeef); size = sizeof(i);
On 3/4/22 20:36, Zebediah Figura wrote:
+ + /* Test option short length. */ + for (i = 0; i < ARRAY_SIZE(test_optsize); ++i) + { + s2 = socket( test_optsize[i].af, test_optsize[i].type, 0 ); + ok(s2 != INVALID_SOCKET, "socket() failed error %d\n", WSAGetLastError()); + for (j = 0; j < 3; ++j)
ARRAY_SIZE(test_optsize[i].sizes)?
The way this test is structured does not make it very easy to read, but I'm struggling to find a better option. The individual sockopts seem very idiosyncratic...
Yes, those behaviour details don't seem to fit any obvious pattern. Maybe I can make it a bit easier if always code the values returned from getsockopt() instead of setting the "default" cases in the code?
Finding a way to include TCP_NODELAY in this table, as well as finding a way to test lengths less than 1 and greater than 4, would probably be helpful. I don't know if you have those in your pending patches.
I am introducing TCP_NODELAY in the same table later, yeah. The reason that it is not here immediately is that TCP_NODELAY needs output length fixup in getsockopt() not to introduce obsucre todo_wine and the remove it, thus a separate patch. I tested greater sizes separately for some options, I will add some tests (hopefully they are all work the same as size 4, at least those that I tried did). Didn't try length < 0 but I will.
+ { + DWORD expected_last_error, expected_value, expected_size, save_value; + int expected_err; + + winetest_push_context("i %u, level %d, optname %d, len %u", + i, test_optsize[i].level, test_optsize[i].optname, 1 << j); + + 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); + + if (test_optsize[i].accepts_short_len || j == 2)
"j == 2" is not great, maybe "size = 1 << j;" earlier, and then "size == 4" here, would be clearer.
+ { + expected_err = 0; + expected_last_error = ERROR_SUCCESS; + expected_size = test_optsize[i].sizes[j] ? test_optsize[i].sizes[j] : 4; + if (test_optsize[i].values[j]) + expected_value = test_optsize[i].values[j]; + else if (expected_size == 4) + expected_value = 1; + else + expected_value = (0xdeadbeef & ~((1 << expected_size * 8) - 1)) | 1;
This is not exactly easy to read. Maybe something like the following would be clearer?
value = 1; expected_value = 0xdeadbeef; memcpy(&expected_value, &value, expected_size);
Yeah, sure.
On 3/4/22 12:04, Paul Gofman wrote:
On 3/4/22 20:36, Zebediah Figura wrote:
+ + /* Test option short length. */ + for (i = 0; i < ARRAY_SIZE(test_optsize); ++i) + { + s2 = socket( test_optsize[i].af, test_optsize[i].type, 0 ); + ok(s2 != INVALID_SOCKET, "socket() failed error %d\n", WSAGetLastError()); + for (j = 0; j < 3; ++j)
ARRAY_SIZE(test_optsize[i].sizes)?
The way this test is structured does not make it very easy to read, but I'm struggling to find a better option. The individual sockopts seem very idiosyncratic...
Yes, those behaviour details don't seem to fit any obvious pattern. Maybe I can make it a bit easier if always code the values returned from getsockopt() instead of setting the "default" cases in the code?
Possibly, yeah, although of course that makes it more annoying to write...
Finding a way to include TCP_NODELAY in this table, as well as finding a way to test lengths less than 1 and greater than 4, would probably be helpful. I don't know if you have those in your pending patches.
I am introducing TCP_NODELAY in the same table later, yeah. The reason that it is not here immediately is that TCP_NODELAY needs output length fixup in getsockopt() not to introduce obsucre todo_wine and the remove it, thus a separate patch.
I tested greater sizes separately for some options, I will add some tests (hopefully they are all work the same as size 4, at least those that I tried did). Didn't try length < 0 but I will.
Sounds great, thanks!
participants (3)
-
Marvin -
Paul Gofman -
Zebediah Figura