Re: ws2_32: Add support and tests for WSARecvMsg and IP_PKTINFO.
On Tue, Oct 12, 2010 at 8:01 PM, Erich Hoover <ehoover(a)mines.edu> 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.
On Tue, Oct 12, 2010 at 10:12 PM, Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> 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 ehoover(a)mines.edu
On Wed, Oct 13, 2010 at 10:30 AM, Erich Hoover <ehoover(a)mines.edu> 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.
participants (2)
-
Erich Hoover -
Mike Kaplinskiy