On Thu, Oct 14, 2010 at 10:08 PM, Erich Hoover <ehoover(a)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(a)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.