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.