[PATCH] ws2_32/tests: Add order-agnostic check in test_simultaneous_async_recv.
Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/ws2_32/tests/sock.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 782b1e59729..966a681e809 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -11520,6 +11520,8 @@ static void test_simultaneous_async_recv(void) const void *expect = msgstr + i * stride; const void *actual = resbuf + i * stride; DWORD size; + int allcmp; + size_t j; ret = WaitForSingleObject(events[i], 1000); ok(!ret, "wait timed out\n"); @@ -11528,6 +11530,20 @@ static void test_simultaneous_async_recv(void) ret = GetOverlappedResult((HANDLE)client, &overlappeds[i], &size, FALSE); ok(ret, "got error %u\n", GetLastError()); ok(size == stride, "got size %u\n", size); + + allcmp = 0; + for (j = 0; j <= num_io * stride - size; j++) allcmp |= !memcmp(msgstr + j, actual, size); + ok(allcmp, "returned data shall be part of original message (got %s)\n", debugstr_an(actual, size)); + + /* Observation: Windows *always* respects the order of WSARecv() calls + * and fills out the buffers in first-in-first-out manner. + * + * This behaviour is admittedly not clearly documented, and it can be + * argued that a well-designed application would instead use vectored + * I/O with an array of WSABUFs, which are then are guaranteed to be + * filled out in order. However, at least one real-world application + * is known to rely on the WSARecv() order-respecting behaviour. + */ ok(!memcmp(expect, actual, stride), "expected %s, got %s\n", debugstr_an(expect, stride), debugstr_an(actual, stride)); } -- 2.31.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=105002 Your paranoid android. === debian11 (32 bit WoW report) === ws2_32: sock.c:295: Test failed: do_synchronous_recv (12c): error 10022: sock.c:707: Test failed: simple_client (12c): received less data than expected: 32704 of 32768 sock.c:712: Test failed: simple_client (12c): test pattern error: 1984
On 1/7/22 12:15, Jinoh Kang wrote:
Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/ws2_32/tests/sock.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 782b1e59729..966a681e809 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -11520,6 +11520,8 @@ static void test_simultaneous_async_recv(void) const void *expect = msgstr + i * stride; const void *actual = resbuf + i * stride; DWORD size; + int allcmp; + size_t j;
ret = WaitForSingleObject(events[i], 1000); ok(!ret, "wait timed out\n"); @@ -11528,6 +11530,20 @@ static void test_simultaneous_async_recv(void) ret = GetOverlappedResult((HANDLE)client, &overlappeds[i], &size, FALSE); ok(ret, "got error %u\n", GetLastError()); ok(size == stride, "got size %u\n", size); + + allcmp = 0; + for (j = 0; j <= num_io * stride - size; j++) allcmp |= !memcmp(msgstr + j, actual, size); + ok(allcmp, "returned data shall be part of original message (got %s)\n", debugstr_an(actual, size)); +
I'm sorry, but this test makes no sense to me. What exactly are you trying to check here?
On 1/9/22 03:37, Zebediah Figura (she/her) wrote:
On 1/7/22 12:15, Jinoh Kang wrote:
Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/ws2_32/tests/sock.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 782b1e59729..966a681e809 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -11520,6 +11520,8 @@ static void test_simultaneous_async_recv(void) const void *expect = msgstr + i * stride; const void *actual = resbuf + i * stride; DWORD size; + int allcmp; + size_t j; ret = WaitForSingleObject(events[i], 1000); ok(!ret, "wait timed out\n"); @@ -11528,6 +11530,20 @@ static void test_simultaneous_async_recv(void) ret = GetOverlappedResult((HANDLE)client, &overlappeds[i], &size, FALSE); ok(ret, "got error %u\n", GetLastError()); ok(size == stride, "got size %u\n", size); + + allcmp = 0; + for (j = 0; j <= num_io * stride - size; j++) allcmp |= !memcmp(msgstr + j, actual, size); + ok(allcmp, "returned data shall be part of original message (got %s)\n", debugstr_an(actual, size)); +
I'm sorry, but this test makes no sense to me. What exactly are you trying to check here?
It's a redundant check meant to ease debugging in case a future regression causes wine to reorder overlapped I/O operations while still completing all of them. It can be taken out if it is deemed unnecessary. Actually, the point of this patch is in the added comment. -- Sincerely, Jinoh Kang
On 1/9/22 06:26, Jinoh Kang wrote:
On 1/9/22 03:37, Zebediah Figura (she/her) wrote:
On 1/7/22 12:15, Jinoh Kang wrote:
Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/ws2_32/tests/sock.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 782b1e59729..966a681e809 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -11520,6 +11520,8 @@ static void test_simultaneous_async_recv(void) const void *expect = msgstr + i * stride; const void *actual = resbuf + i * stride; DWORD size; + int allcmp; + size_t j; ret = WaitForSingleObject(events[i], 1000); ok(!ret, "wait timed out\n"); @@ -11528,6 +11530,20 @@ static void test_simultaneous_async_recv(void) ret = GetOverlappedResult((HANDLE)client, &overlappeds[i], &size, FALSE); ok(ret, "got error %u\n", GetLastError()); ok(size == stride, "got size %u\n", size); + + allcmp = 0; + for (j = 0; j <= num_io * stride - size; j++) allcmp |= !memcmp(msgstr + j, actual, size); + ok(allcmp, "returned data shall be part of original message (got %s)\n", debugstr_an(actual, size)); +
I'm sorry, but this test makes no sense to me. What exactly are you trying to check here?
It's a redundant check meant to ease debugging in case a future regression causes wine to reorder overlapped I/O operations while still completing all of them.
It can be taken out if it is deemed unnecessary. Actually, the point of this patch is in the added comment.
I'm not sure I see the need for this additional test. If we complete the asyncs out of order we'll already see the original message dumped out of order; the existing test already uses debugstr_an().
On 1/11/22 06:00, Zebediah Figura (she/her) wrote:
On 1/9/22 06:26, Jinoh Kang wrote:
On 1/9/22 03:37, Zebediah Figura (she/her) wrote:
On 1/7/22 12:15, Jinoh Kang wrote:
Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/ws2_32/tests/sock.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 782b1e59729..966a681e809 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -11520,6 +11520,8 @@ static void test_simultaneous_async_recv(void) const void *expect = msgstr + i * stride; const void *actual = resbuf + i * stride; DWORD size; + int allcmp; + size_t j; ret = WaitForSingleObject(events[i], 1000); ok(!ret, "wait timed out\n"); @@ -11528,6 +11530,20 @@ static void test_simultaneous_async_recv(void) ret = GetOverlappedResult((HANDLE)client, &overlappeds[i], &size, FALSE); ok(ret, "got error %u\n", GetLastError()); ok(size == stride, "got size %u\n", size); + + allcmp = 0; + for (j = 0; j <= num_io * stride - size; j++) allcmp |= !memcmp(msgstr + j, actual, size); + ok(allcmp, "returned data shall be part of original message (got %s)\n", debugstr_an(actual, size)); +
I'm sorry, but this test makes no sense to me. What exactly are you trying to check here?
It's a redundant check meant to ease debugging in case a future regression causes wine to reorder overlapped I/O operations while still completing all of them.
It can be taken out if it is deemed unnecessary. Actually, the point of this patch is in the added comment.
I'm not sure I see the need for this additional test. If we complete the asyncs out of order we'll already see the original message dumped out of order; the existing test already uses debugstr_an().
Fair point, if we don't need the extra test then I guess we can mark it rejected. -- Sincerely, Jinoh Kang
participants (3)
-
Jinoh Kang -
Marvin -
Zebediah Figura (she/her)