[PATCH] ws2_32: Don't post completion packet if receiving fails with error immediately.

[Stefan Dösinger]

> Am 09.05.2017 um 01:43 schrieb Bruno Jesus <00cpxxx at gmail.com>:
> 
> Awesome work, thanks ;-)
> Some unused SetLastError calls and since I'm sending an email why not
> complain more?
I largely followed the style of the surrounding code. It is very defensive with seemingly redundant calls, yes.

I spent 2 hours trying to figure out why Windows gave me a weird 1024 byte read on the second read (the graceful close one, which should return success with 0 bytes), only to figure out that it happened because I didn’t memset the overlapped structure back to zero. At that point I decided to keep the defensive style of the rest of the test.

I can certainly take another shot at removing things, just don’t blame me that it might affect the behavior of the existing tests in this function :-)

> Some winsock functions are basically boolean where 0 is true and
> SOCKET_ERROR is false, so (I believe) in most other places we use
> "WSASend failed with error %d", GetLastError()). This would require a
> previous SetLastError.
Following existing style here.

>> +    ok(num_bytes == sizeof(buf), "Managed to send %d\n", num_bytes);
> 
> I believe most code in ws2_32 prefer "Expected X, got Y\n“.
Following existing style, and in d3d we generally avoid ok(result == ABC "expected ABC, but hell froze over"); lines because it specifies the expected results twice and is begging for inconsistency. To understand what is going wrong you need to look at the test code anyway.

> Better to remove the extra new empty line as in the other use of Sleep
> there is no empty line. Besides it makes it harder for AJ to see you
> are adding sleep to tests :-)
I haven’t checked if the newlines follow existing style, I’ll double check. The existing code already has the sleeps. I am not sure if they are necessary, but if removing them introduces random failures we may or may not catch it immediately.

> AFAIR this is not your fault, it is a bug. WSARecv has a particular
> behavior of not returning a "0 bytes read", instead it returns error
> with WSAECONNRESET. Wine simply returns success with 0 bytes, which
> stands for connection reset in the old recv() way. This explains the 3
> todo_wine below.
No, Windows does return 0 bytes on a graceful close (see the 2nd of my 3 tests). On Wine we do get WSAECONNRESET in some situations. These WSAECONNRESET returns are what triggers the crash in libtorrent. On Windows I can trigger WSAECONNRESET on the receiving end with the SO_LINGER option + closesocket on the sending end. That doesn’t work on my Linux box, although various internet pages suggest it should work.

As far as I understand WSAECONNRESET (or EPIPE in POSIX) 00is returned when the remote end rebooted, had power cut or got physically disconnected - which are valid situations, but a bit difficult to trigger from a test. But because libtorrent connects to loads of computers across the internet, having one peer go out of wifi range or run out of battery isn’t unexpected.

>> +    iret = WSARecv(dest, &bufs, 1, &num_bytes, &flags, &ov, NULL);
>> +    todo_wine ok(iret == SOCKET_ERROR, "WSARecv failed - %d\n", iret);
>> +    todo_wine ok(GetLastError() == WSAECONNRESET, "Last error was %d\n", GetLastError());
>> +    todo_wine ok(num_bytes == 0xdeadbeef, "Managed to send %d\n", num_bytes);
> 
> Expected/got format? If not the sentence is wrong: send/recv.
Whoops on the send/recv :-) .