Re: [PATCH 2/4] ws2_32/tests: Test ICMP.

[Zebediah Figura]

On 7/5/22 14:53, Paul Gofman wrote:
> From: Paul Gofman
> Signed-off-by: Paul Gofman
>
> dlls/ws2_32/tests/sock.c | 150 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 150 insertions(+)

I don't have much familiarity with ICMP, but...
> +static void test_icmp(const char *dest_addr)

Any reason to pass this as a parameter? I'm guessing it's convenient to 
test ping over a real adapter, but presumably it can also just be edited 
inside the function...
> +{
> - static const unsigned int ping_data = 0xdeadbeef;
> - struct icmp_hdr *icmp_h;
> - BYTE send_buf[sizeof(struct icmp_hdr) + sizeof(ping_data)];
> - UINT16 recv_checksum, checksum;
> - unsigned int reply_data;
> - struct sockaddr_in sa;
> - struct ip_hdr *ip_h;
> - struct in_addr addr;
> - BYTE recv_buf[256];
> - SOCKET s;
> - int ret;
> -
> - s = WSASocketA(AF_INET, SOCK_RAW, IPPROTO_ICMP, NULL, 0, 0);
> - if (s == INVALID_SOCKET)
> - {
> -
>    ret = WSAGetLastError();
> -
>    ok(ret == WSAEACCES, "Expected 10013, received %d\n", ret);
> -
>    skip("SOCK_RAW is not supported\n");
> -
>    return;
> - }

To clarify, this should never trip on Windows machines; it's only here 
for Linux machines that don't support unprivileged ICMP, right? If so a 
comment to that effect would be helpful. I also don't know if we want to 
use a todo_wine in that case instead of skip().
> -
> - icmp_h = (struct icmp_hdr *)send_buf;
> - icmp_h->type = ICMP4_ECHO_REQUEST;
> - icmp_h->code = 0;
> - icmp_h->checksum = 0;
> - icmp_h->un.echo.id = 0xbeaf; /* will be overwritten for linux ping socks */
> - icmp_h->un.echo.sequence = 1;
> - *(unsigned int *)(icmp_h + 1) = ping_data;
> - icmp_h->checksum = chksum(send_buf, sizeof(send_buf));
> -
> - memset(&sa, 0, sizeof(sa));
> - sa.sin_family = AF_INET;
> - sa.sin_port = 0;
> - sa.sin_addr.s_addr = inet_addr(dest_addr);
> -
> - ret = sendto(s, (char *)send_buf, sizeof(send_buf), 0, (struct sockaddr*)&sa, sizeof(sa));
> - ok(ret != SOCKET_ERROR, "got error %d.\n", WSAGetLastError());

Any reason not to just say "ok(ret == sizeof(send_buf)"?