Re: ws2_32: Add support and tests for WSARecvMsg and IP_PKTINFO.

[Mike Kaplinskiy]

On Tue, Oct 12, 2010 at 8:01 PM, Erich Hoover  wrote:
> Real Name:
>     Erich Hoover
> Description:
>     While searching for something else I discovered a bug relevant to the
> interface-bound UDP broadcast patches I've been working on.  Apparently,
> IP_PKTINFO does work on Windows when used with the special WSARecvMsg
> function (Bug #19493).  The attached patch addresses this issue by
> implementing WSARecvMsg through WS2_recvfrom and by converting the Unix
> IP_PKTINFO response to the Windows equivalent.
> ChangeLog:
>     ws2_32: Add support and tests for WSARecvMsg and IP_PKTINFO.

Sorry I must have missed the RFC when reading my mail. These comments
are purely mine, and may not have any value for getting your patch in
:).

+static inline WSACMSGHDR *create_control_message(int level, int type,
WSACMSGHDR *current, ULONG *maxsize, void *data, int len)
This doesn't seem like a "create", more like a "fill" or "write".

+    int newsize = (int)*maxsize;
+
+    /* Make sure there is at least enough room for this entry */
+    newsize -= sizeof(WSACMSGHDR) + CMSG_ALIGN(len);
+    if (newsize < 0)
+        return 0;
+    *maxsize = (ULONG)newsize;
Just declare it as a ULONG/size_t so you don't have to cast.

+static inline int convert_control_headers(struct msghdr *hdr, ULONG *maxsize)
You shouldn't mix using memory as unix-style CMSGs and windows-style
CMSGs. Just allocate something to store the unix messages, read them
in and convert them into the user-passed buffer. This seems similar to
what we do for address conversion.

+    memset(cmsg_win, 0x00, sizeof(WSACMSGHDR)); /* Don't use garbage
data if no headers are found */
I think in general that is discouraged at wine (don't quote me on
that). You should just initialize the values manually.

+    if (wsa->control && !convert_control_headers(&hdr, wsa->controllen))
+    {
+        /* Insufficient room for control headers */
+        errno = EINVAL;
+        return -1;
+    }
This should probably be in an #ifdef of some sort.

+    msg->dwFlags |= WINE_MSG_HASCTRL;
+    memcpy(buftmp, msg->lpBuffers, msg->dwBufferCount * sizeof(WSABUF));
+    buftmp[msg->dwBufferCount] = msg->Control;
+    ret = WS2_recvfrom( s, buftmp, msg->dwBufferCount, lpNumberOfBytesRecvd,
+                        &msg->dwFlags, msg->name, &msg->namelen,
+                        lpOverlapped, lpCompletionRoutine );
You shouldn't add internal flags like that. Just rewrite WS2_recvfrom
to allow returning message headers instead of hacking around it.

+static int    (WINAPI
*pWSARecvMsg)(SOCKET,LPWSAMSG,LPDWORD,LPWSAOVERLAPPED,LPWSAOVERLAPPED_COMPLETION_ROUTINE)
= 0;
+GUID WSARecvMsg_GUID = WSAID_WSARECVMSG;
For one reason or another the convention is to get it during your test
function (see AcceptEx & ConnectEx). (This does have some merit as
SIO_GET_EXTENSION_FUNCTION_POINTER can vary with the socket that's
passed)

+    s1addr.sin_family      = AF_INET;
+    s1addr.sin_port        = htons(9375);
+    s2addr.sin_family      = AF_INET;
+    s2addr.sin_port        = htons(9375);
You should probably bind to 0 and then use getsockname to get the
actual port you bound to.

+        s1=socket(AF_INET, SOCK_DGRAM, 0);
+        ok(s1!=INVALID_SOCKET, "socket() failed error: %d\n",
WSAGetLastError());
Spaces around = & != would make me happy (and below). Also you don't
error out correctly if socket creation fails.

diff --git a/include/mswsock.h b/include/mswsock.h
index 322ab20..5749e59 100644
--- a/include/mswsock.h
+++ b/include/mswsock.h
This should be a separate patch.

diff --git a/include/ws2ipdef.h b/include/ws2ipdef.h
index 11b3689..ec0b85a 100644
--- a/include/ws2ipdef.h
+++ b/include/ws2ipdef.h
Same here.

Mike.

ws2_32: Add support and tests for WSARecvMsg and IP_PKTINFO.

[Erich Hoover]

On Tue, Oct 12, 2010 at 10:12 PM, Mike Kaplinskiy
 wrote:
> ...
> - int newsize = (int)*maxsize;
> -
> - /* Make sure there is at least enough room for this entry */
> - newsize -= sizeof(WSACMSGHDR) + CMSG_ALIGN(len);
> - if (newsize < 0)
> - return 0;
> - *maxsize = (ULONG)newsize;
>
> Just declare it as a ULONG/size_t so you don't have to cast.

I can obviously change this around a little bit, but the reason for
this conversion is that as a ULONG the "if (newsize < 0)" comparison
is not going to work.
> ...
> - memset(cmsg_win, 0x00, sizeof(WSACMSGHDR)); /* Don't use garbage
>
> data if no headers are found */
> I think in general that is discouraged at wine (don't quote me on
> that). You should just initialize the values manually.

This is actually superfluous (gone in next revision), as if no headers
are found the length returned is zero.
> - msg->dwFlags |= WINE_MSG_HASCTRL;
> - memcpy(buftmp, msg->lpBuffers, msg->dwBufferCount * sizeof(WSABUF));
> - buftmp[msg->dwBufferCount] = msg->Control;
> - ret = WS2_recvfrom( s, buftmp, msg->dwBufferCount, lpNumberOfBytesRecvd,
> - &msg->dwFlags, msg->name, &msg->namelen,
> - lpOverlapped, lpCompletionRoutine );
>
> You shouldn't add internal flags like that. Just rewrite WS2_recvfrom
> to allow returning message headers instead of hacking around it.

Is it acceptable to just add a parameter for WS2_recvfrom() or should
this function get renamed?  This function currently mirrors
WSARecvFrom exactly.
> - s1=socket(AF_INET, SOCK_DGRAM, 0);
> - ok(s1!=INVALID_SOCKET, "socket() failed error: %d\n",
>
> WSAGetLastError());
> Spaces around = & != would make me happy (and below). Also you don't
> error out correctly if socket creation fails.

I was trying to match test_so_reuseaddr(), I'll change this.

Erich Hoover
[e-mail]

ws2_32: Add support and tests for WSARecvMsg and IP_PKTINFO.

[Mike Kaplinskiy]

On Wed, Oct 13, 2010 at 10:30 AM, Erich Hoover  wrote:
> Is it acceptable to just add a parameter for WS2_recvfrom() or should
> this function get renamed?  This function currently mirrors
> WSARecvFrom exactly.

Feel free to change the name if you want, I don't have any problems
with it (although I can't speak for Alexandre).

Mike.