On Tue, Oct 13, 2010 at 16:30 PM, Erich Hoover ehoover@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
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 ehoover@mines.edu
On Thu, Oct 14, 2010 at 10:08 PM, Erich Hoover ehoover@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 ehoover@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 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.
- /* 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.
On Sat, Oct 16, 2010 at 1:41 PM, Mike Kaplinskiy mike.kaplinskiy@gmail.com wrote:
On Thu, Oct 14, 2010 at 10:08 PM, Erich Hoover ehoover@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_PK... [2/4] include: Add macros for retrieving control message headers. http://www.compholio.com/wine-kane/patches/2010-10-17/0002-include-Add-macro... [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-suppor... [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-...
Erich Hoover ehoover@mines.edu
On Sun, Oct 17, 2010 at 5:51 PM, Erich Hoover ehoover@mines.edu wrote:
On Sat, Oct 16, 2010 at 1:41 PM, Mike Kaplinskiy mike.kaplinskiy@gmail.com wrote:
On Thu, Oct 14, 2010 at 10:08 PM, Erich Hoover ehoover@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.
On Mon, Oct 18, 2010 at 8:58 PM, Mike Kaplinskiy mike.kaplinskiy@gmail.com wrote:
On Sun, Oct 17, 2010 at 5:51 PM, Erich Hoover ehoover@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-suppor... [4/4] http://www.compholio.com/wine-kane/patches/2010-10-20/0004-ws2_32-tests-Add-...
Erich Hoover ehoover@mines.edu
-
Erich Hoover -
Mike Kaplinskiy -
Rolf Kalbermatter