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

2010-10-20 Thread Erich Hoover 
On Mon, Oct 18, 2010 at 8:58 PM, Mike Kaplinskiy
mike.kaplins...@gmail.com wrote:
 On Sun, Oct 17, 2010 at 5:51 PM, Erich Hoover ehoo...@mines.edu wrote:
 ...
 That's why in the original version I had those parameters split out;
 however, I realized that since the length parameter gets modified by
 WSARecvMsg that it is impossible for the WSAMSG structure (or any of
 its parameters) to disappear before WSARecvMsg completes.  I have
 re-written the tests (see patch 4 at the end) to show that this
 structure gets modified even when an overlapped request is used and
 results in modification of the structure at a later time, the results
 from the test bot can be found here:
    https://testbot.winehq.org/JobDetails.pl?Key=6250

 Fair enough. Would you mind adding a test to see if the flags are
 changed as well (and see if the flags returned by
 WSAGetOverlappedResult are of that sort as well)? If it doesn't you
 can drop lpFlags and just copy it from flags. Otherwise you may want
 to rename it to be less windows-y (like local_flags and flags).


I'm not entirely following you here, but if I understand you correctly
then the lpFlags is appropriate.  The flags in the WSAMSG structure
get modified while the WSAGetOverlappedResult flags do not (which
works as is since the overlapped version of the flags is not a
pointer).  I've changed the patch around a little so that the pointer
version is only used when appropriate (and added tests for this
functionality):
[3/4] 
http://www.compholio.com/wine-kane/patches/2010-10-20/0003-ws2_32-Add-support-for-WSARecvMsg-and-IP_PKTINFO.patch
[4/4] 
http://www.compholio.com/wine-kane/patches/2010-10-20/0004-ws2_32-tests-Add-regression-tests-for-WSARecvMsg-and.patch

Erich Hoover
ehoo...@mines.edu

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

2010-10-18 Thread Mike Kaplinskiy 
On Sun, Oct 17, 2010 at 5:51 PM, Erich Hoover ehoo...@mines.edu wrote:
 On Sat, Oct 16, 2010 at 1:41 PM, Mike Kaplinskiy
 mike.kaplins...@gmail.com wrote:
 On Thu, Oct 14, 2010 at 10:08 PM, Erich Hoover ehoo...@mines.edu wrote:
 ...
     DWORD                               flags;
     unsigned int                        n_iovecs;
     unsigned int                        first_iovec;
+    WSABUF                             *control;
     struct iovec                        iovec[1];
 } ws2_async;
 I would prefer control declared above n_iovecs purely for
 organizational purposes (params first, buffers and info about them
 last). Also you can't just store the passed in WSABUF pointer; it may
 be stack allocated and may disappear before the overlapped call
 completes. You could just add control and control_len instead of using
 WSABUF.

 That's why in the original version I had those parameters split out;
 however, I realized that since the length parameter gets modified by
 WSARecvMsg that it is impossible for the WSAMSG structure (or any of
 its parameters) to disappear before WSARecvMsg completes.  I have
 re-written the tests (see patch 4 at the end) to show that this
 structure gets modified even when an overlapped request is used and
 results in modification of the structure at a later time, the results
 from the test bot can be found here:
    https://testbot.winehq.org/JobDetails.pl?Key=6250

Fair enough. Would you mind adding a test to see if the flags are
changed as well (and see if the flags returned by
WSAGetOverlappedResult are of that sort as well)? If it doesn't you
can drop lpFlags and just copy it from flags. Otherwise you may want
to rename it to be less windows-y (like local_flags and flags).

+        /* Insufficient room for control headers */
+        errno = EINVAL;
 This should probably be ENOMEM then. On that note, it would be nice to
 test what windows actually does when you don't give it enough space
 (like 1 byte). We should probably rewrite that function at some point
 since I'm not sure what errno it returns anymore.

 This is odd, I must have copy-pasted the wrong error code from the
 recvmsg() manual page.  I've investigated this issue more and
 discovered that the code should actually be EMSGSIZE.  This change and
 the corresponding tests can be found in my revised patches (at the
 end), I've also included support for reporting back the MSG_CTRUNC
 flag.

Hmm, you may also want to warn/err on EMSGSIZE coming from recvmsg.

+#ifdef IP_PKTINFO
+        case WS_IP_PKTINFO:
+#endif
 You probably don't need the ifdef. convert_sockopt will just fail.

 I did this to match how IP_HDRINCL was done nearby, should this really
 be changed?

We seem to mix both styles. I don't have any preference then.

Mike.

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

2010-10-17 Thread Erich Hoover 
On Sat, Oct 16, 2010 at 1:41 PM, Mike Kaplinskiy
mike.kaplins...@gmail.com wrote:
 On Thu, Oct 14, 2010 at 10:08 PM, Erich Hoover ehoo...@mines.edu wrote:
 ...
     DWORD                               flags;
     unsigned int                        n_iovecs;
     unsigned int                        first_iovec;
+    WSABUF                             *control;
     struct iovec                        iovec[1];
 } ws2_async;
 I would prefer control declared above n_iovecs purely for
 organizational purposes (params first, buffers and info about them
 last). Also you can't just store the passed in WSABUF pointer; it may
 be stack allocated and may disappear before the overlapped call
 completes. You could just add control and control_len instead of using
 WSABUF.

That's why in the original version I had those parameters split out;
however, I realized that since the length parameter gets modified by
WSARecvMsg that it is impossible for the WSAMSG structure (or any of
its parameters) to disappear before WSARecvMsg completes.  I have
re-written the tests (see patch 4 at the end) to show that this
structure gets modified even when an overlapped request is used and
results in modification of the structure at a later time, the results
from the test bot can be found here:
https://testbot.winehq.org/JobDetails.pl?Key=6250

+        /* Insufficient room for control headers */
+        errno = EINVAL;
 This should probably be ENOMEM then. On that note, it would be nice to
 test what windows actually does when you don't give it enough space
 (like 1 byte). We should probably rewrite that function at some point
 since I'm not sure what errno it returns anymore.

This is odd, I must have copy-pasted the wrong error code from the
recvmsg() manual page.  I've investigated this issue more and
discovered that the code should actually be EMSGSIZE.  This change and
the corresponding tests can be found in my revised patches (at the
end), I've also included support for reporting back the MSG_CTRUNC
flag.

+#ifdef IP_PKTINFO
+        case WS_IP_PKTINFO:
+#endif
 You probably don't need the ifdef. convert_sockopt will just fail.

I did this to match how IP_HDRINCL was done nearby, should this really
be changed?

+    hdr.Control.buf = pktbuf;
 .len = sizeof(pktbuf)?

The length parameter is declared within the loop, otherwise it gets
resized (as mentioned earlier) when WSARecvMsg is called.

Thank you so much for all your help, I hope you find the following to
be more to your liking.

[1/4] include: Add IP_PKTINFO response structure.

http://www.compholio.com/wine-kane/patches/2010-10-17/0001-include-Add-IP_PKTINFO-response-structure.patch
[2/4] include: Add macros for retrieving control message headers.

http://www.compholio.com/wine-kane/patches/2010-10-17/0002-include-Add-macros-for-retrieving-control-message-he.patch
[3/4] ws2_32: Add support for WSARecvMsg and IP_PKTINFO.

http://www.compholio.com/wine-kane/patches/2010-10-17/0003-ws2_32-Add-support-for-WSARecvMsg-and-IP_PKTINFO.patch
[4/4] ws2_32/tests: Add regression tests for WSARecvMsg and IP_PKTINFO.

http://www.compholio.com/wine-kane/patches/2010-10-17/0004-ws2_32-tests-Add-regression-tests-for-WSARecvMsg-and.patch

Erich Hoover
ehoo...@mines.edu

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

2010-10-16 Thread Mike Kaplinskiy 
On Thu, Oct 14, 2010 at 10:08 PM, Erich Hoover ehoo...@mines.edu wrote:
 If you guys would mind looking over this revised patchset I would
 greatly appreciate it, I believe I've appropriately included your
 corrections.  Please note that mswsock.h and ws2ipdef.h will be
 submitted as separate patches.  Thanks in advance!

 Erich Hoover
 ehoo...@mines.edu


I still have a few comments on the patch, but all of them are minor.
 - You should have the test in a separate patch as well.

 DWORD   flags;
 unsigned intn_iovecs;
 unsigned intfirst_iovec;
+WSABUF *control;
 struct ioveciovec[1];
 } ws2_async;
I would prefer control declared above n_iovecs purely for
organizational purposes (params first, buffers and info about them
last). Also you can't just store the passed in WSABUF pointer; it may
be stack allocated and may disappear before the overlapped call
completes. You could just add control and control_len instead of using
WSABUF.

+/* Loop over all the headers, converting as appropriate */
+struct cmsghdr *cmsg_unix = (struct cmsghdr *) CMSG_FIRSTHDR(hdr);
...
+for (ptr = cmsg_win; cmsg_unix != NULL; cmsg_unix = CMSG_NXTHDR(hdr, 
cmsg_unix))
That's just confusing. Please initialize cmsg_unix in the for loop and
ptr outside the for loop.

+/* Insufficient room for control headers */
+errno = EINVAL;
This should probably be ENOMEM then. On that note, it would be nice to
test what windows actually does when you don't give it enough space
(like 1 byte). We should probably rewrite that function at some point
since I'm not sure what errno it returns anymore.

+if (!msg) return SOCKET_ERROR;
That doesn't seem right to me (no SetLastError?). Is there a test for this?

+#ifdef IP_PKTINFO
+case WS_IP_PKTINFO:
+#endif
You probably don't need the ifdef. convert_sockopt will just fail.

+hdr.Control.buf = pktbuf;
.len = sizeof(pktbuf)?

+if (!pWSARecvMsg)
+{
+skip(WSARecvMsg is unsupported, some tests will be skipped.\n);
+return;
+}
You probably want win_skip. You also leak some sockets here.

Mike.

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

2010-10-14 Thread Rolf Kalbermatter 
On Tue, Oct 13, 2010 at 16:30 PM, Erich Hoover ehoo...@mines.edu 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.

Then following code would work better:

ULONG newsize = sizeof(WSACMSGHDR) + CMSG_ALIGN(len);

/* Make sure there is at least enough room for this entry */
if (newsize  *maxsize)
return 0;
/* else */
   *maxsize -= newsize;



Rolf Kalbermatter

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

2010-10-14 Thread Erich Hoover 
If you guys would mind looking over this revised patchset I would
greatly appreciate it, I believe I've appropriately included your
corrections.  Please note that mswsock.h and ws2ipdef.h will be
submitted as separate patches.  Thanks in advance!

Erich Hoover
ehoo...@mines.edu
diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c
index 22e6138..880db95 100644
--- a/dlls/ws2_32/socket.c
+++ b/dlls/ws2_32/socket.c
@@ -171,9 +171,9 @@ WINE_DECLARE_DEBUG_CHANNEL(winediag);
 
 
 /*
- * The actual definition of WSASendTo/WSARecvFrom, wrapped in a different
- * function name, so that internal calls from ws2_32 itself will not trigger
- * programs like Garena, which hooks WSASendTo/WSARecvFrom calls.
+ * The actual definition of WSASendTo, wrapped in a different function name
+ * so that internal calls from ws2_32 itself will not trigger programs like
+ * Garena, which hooks WSASendTo/WSARecvFrom calls.
  */
 static int WS2_sendto( SOCKET s, LPWSABUF lpBuffers, DWORD dwBufferCount,
LPDWORD lpNumberOfBytesSent, DWORD dwFlags,
@@ -181,11 +181,16 @@ static int WS2_sendto( SOCKET s, LPWSABUF lpBuffers, DWORD dwBufferCount,
LPWSAOVERLAPPED lpOverlapped,
LPWSAOVERLAPPED_COMPLETION_ROUTINE lpCompletionRoutine );
 
-static int WS2_recvfrom( SOCKET s, LPWSABUF lpBuffers, DWORD dwBufferCount,
- LPDWORD lpNumberOfBytesRecvd, LPDWORD lpFlags,
- struct WS_sockaddr *lpFrom,
- LPINT lpFromlen, LPWSAOVERLAPPED lpOverlapped,
- LPWSAOVERLAPPED_COMPLETION_ROUTINE lpCompletionRoutine );
+/*
+ * Internal fundamental receive function, essentially WSARecvFrom with an
+ * additional parameter to support message control headers.
+ */
+static int WS2_recv_base( SOCKET s, LPWSABUF lpBuffers, DWORD dwBufferCount,
+  LPDWORD lpNumberOfBytesRecvd, LPDWORD lpFlags,
+  struct WS_sockaddr *lpFrom,
+  LPINT lpFromlen, LPWSAOVERLAPPED lpOverlapped,
+  LPWSAOVERLAPPED_COMPLETION_ROUTINE lpCompletionRoutine,
+  LPWSABUF lpControlBuffer );
 
 /* critical section to protect some non-reentrant net function */
 static CRITICAL_SECTION csWSgetXXXbyYYY;
@@ -263,6 +268,7 @@ typedef struct ws2_async
 DWORD   flags;
 unsigned intn_iovecs;
 unsigned intfirst_iovec;
+WSABUF *control;
 struct ioveciovec[1];
 } ws2_async;
 
@@ -375,6 +381,9 @@ static const int ws_ip_map[][2] =
 #endif
 MAP_OPTION( IP_TOS ),
 MAP_OPTION( IP_TTL ),
+#ifdef IP_PKTINFO
+MAP_OPTION( IP_PKTINFO ),
+#endif
 };
 
 static const int ws_ipv6_map[][2] =
@@ -470,6 +479,71 @@ static const int ws_eai_map[][2] =
 
 static const char magic_loopback_addr[] = {127, 12, 34, 56};
 
+#ifndef HAVE_STRUCT_MSGHDR_MSG_ACCRIGHTS
+static inline WSACMSGHDR *fill_control_message(int level, int type, WSACMSGHDR *current, ULONG *maxsize, void *data, int len)
+{
+ULONG msgsize = sizeof(WSACMSGHDR) + WSA_CMSG_ALIGN(len);
+char *ptr = (char *) current + sizeof(WSACMSGHDR);
+
+/* Make sure there is at least enough room for this entry */
+if (msgsize  *maxsize)
+return 0;
+*maxsize -= msgsize;
+/* Fill in the entry */
+current-cmsg_len = sizeof(WSACMSGHDR) + len;
+current-cmsg_level = level;
+current-cmsg_type = type;
+memcpy(ptr, data, len);
+/* Return the pointer to where next entry should go */
+return (WSACMSGHDR *) (ptr + WSA_CMSG_ALIGN(len));
+}
+
+static inline int convert_control_headers(struct msghdr *hdr, WSABUF *control)
+{
+struct cmsghdr *cmsg_unix = (struct cmsghdr *) CMSG_FIRSTHDR(hdr);
+WSACMSGHDR *cmsg_win = (WSACMSGHDR *) control-buf, *ptr;
+ULONG ctlsize = control-len;
+
+/* Loop over all the headers, converting as appropriate */
+for (ptr = cmsg_win; cmsg_unix != NULL; cmsg_unix = CMSG_NXTHDR(hdr, cmsg_unix))
+{
+switch(cmsg_unix-cmsg_level)
+{
+case IPPROTO_IP:
+switch(cmsg_unix-cmsg_type)
+{
+#ifdef IP_PKTINFO
+case IP_PKTINFO:
+{
+/* Convert the Unix IP_PKTINFO structure to the Windows version */
+struct in_pktinfo *data_unix = (struct in_pktinfo *) CMSG_DATA(cmsg_unix);
+struct WS_in_pktinfo data_win;
+
+memcpy(data_win.ipi_addr,data_unix-ipi_addr.s_addr,4); /* 4 bytes = 32 address bits */
+data_win.ipi_ifindex = data_unix-ipi_ifindex;
+ptr = fill_control_message(WS_IPPROTO_IP, WS_IP_PKTINFO, ptr, ctlsize,
+   (void*)data_win, sizeof(data_win));
+if (!ptr)

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

2010-10-13 Thread Erich Hoover 
On Tue, Oct 12, 2010 at 10:12 PM, Mike Kaplinskiy
mike.kaplins...@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
ehoo...@mines.edu

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

2010-10-13 Thread Mike Kaplinskiy 
On Wed, Oct 13, 2010 at 10:30 AM, Erich Hoover ehoo...@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.

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

2010-10-12 Thread Mike Kaplinskiy 
On Tue, Oct 12, 2010 at 8:01 PM, Erich Hoover ehoo...@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.