Thanks for the review. Replies under <LS>. Linda
----- Original Message ----- From: "Ben Pfaff" <[email protected]> To: "Linda Sun" <[email protected]> Cc: [email protected] Sent: Tuesday, December 17, 2013 4:46:44 PM Subject: Re: [ovs-dev] [PATCH] pool-loop: windows poll_block implementation On Thu, Dec 12, 2013 at 04:28:12PM -0800, Linda Sun wrote: > Use WaitForMultipleObjects for polling on windows. This works on all kinds > of objects, e.g. sockets, files, especially ioctl calls to the kernel. > One additional paramater is passed down in poll_fd_wait() to help > WaitForMultipleObjects. > latch is signaled with event, to be waited/polled by WaitForMultipleObjects() > as well. > Changed array of fds to hmap to check for duplicate fds. > > Signed-off-by: Linda Sun <[email protected]> Hi Linda, thanks for the second version. GCC reports: ../lib/poll-loop.c: In function 'poll_block': ../lib/poll-loop.c:304:16: error: unused variable 'i' [-Werror=unused-variable] ../lib/timeval.c: In function 'time_poll': ../lib/timeval.c:235:56: error: unused parameter 'handles' [-Werror=unused-parameter] <LS> Sorry didn't realize that warning doesn't fail the build and I didn't check the make log. I'll fix these. ../lib/stream-ssl.c: In function 'ssl_run_wait': ../lib/stream-ssl.c:694:66: error: macro "poll_fd_wait" requires 3 arguments, but only 2 given ../lib/stream-ssl.c:694:9: error: 'poll_fd_wait' undeclared (first use in this function) ../lib/stream-ssl.c:694:9: note: each undeclared identifier is reported only once for each function it appears in ../lib/stream-ssl.c: In function 'ssl_wait': ../lib/stream-ssl.c:710:47: error: macro "poll_fd_wait" requires 3 arguments, but only 2 given ../lib/stream-ssl.c:710:17: error: 'poll_fd_wait' undeclared (first use in this function) ../lib/stream-ssl.c:717:70: error: macro "poll_fd_wait" requires 3 arguments, but only 2 given ../lib/stream-ssl.c:728:70: error: macro "poll_fd_wait" requires 3 arguments, but only 2 given ../lib/stream-ssl.c: In function 'pssl_wait': ../lib/stream-ssl.c:854:34: error: macro "poll_fd_wait" requires 3 arguments, but only 2 given ../lib/stream-ssl.c:854:5: error: 'poll_fd_wait' undeclared (first use in this function) ../lib/stream-ssl.c:853:26: error: unused variable 'pssl' [-Werror=unused-variable] ../lib/stream-ssl.c: At top level: ../lib/stream-ssl.c:188:1: error: 'want_to_poll_events' defined but not used [-Werror=unused-function] <LS> How do I make with the ssl option? I completely missed these. The subject should be "poll" loop, not "pool" loop. I see that latch.c is completely different on Windows and Linux. I'd rather have two separate sources files, then, such as latch-windows.c and latch-unix.c, rather than one file with piles of ifdefs. I see that Windows also wants different members in struct latch; I guess an #ifdef in the header file is a reasonable way to do that. Almost every caller of poll_fd_wait() passes 0 for wevent. I suggest adding a new function for nonzero wevent rather than confusing unix-oriented code readers. Either way, the comment on the function that takes the wevent argument needs to explain what it means. wevent is a uint32_t but poll_fd_wait_at() compares it to NULL. <LS> actually wevent is a HANDLE on windows. What's your preference, define HANDLE on unix platform, pass a void * and cast it? I'm ok either way. Now that we're using an hmap, there's no need to keep an 'n_waiters' member in poll_loop. (We can just use hmap_count(&loop->poll_nodes).) Can we avoids allocating memory for wevents entirely outside of Windows platforms? <LS> passing NULL OK with you? It looks like time_poll() is trying to return a Windows error number as if it were an errno value. I think that's going to cause trouble. Please figure out some way to return an appropriate errno value. <LS> I have a win_error_to_errno() function in dpif-windows.c. I can through that in. It's a work in progress, but it has a few windows errors converted to errno so far. Linda Thanks, Ben _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
