On Tue, Sep 29, 2015 at 7:44 PM, Alin Serdean <[email protected]> wrote: > The ides is the following: > https://msdn.microsoft.com/en-us/library/windows/desktop/ms740094%28v=vs.85%29.aspx In the same documentation I don't understand why they say: "The identifier of the socket for which to find status. This parameter is ignored if set to a negative value".
That statement made me think that it can actually take -ve value. > > When we do the following: > node->pollfd.fd = fd; > and fd = -1, node->pollfd.fd will rollover. > > Another aspect to the problem is that we can insert multiple times the same > wevent into the map. According to the documentation: > https://msdn.microsoft.com/en-us/library/windows/desktop/ms687025%28v=vs.85%29.aspx > it should not. > We should probably split: > https://github.com/openvswitch/ovs/blob/master/lib/poll-loop.c#L69-L71 > Into two: > node->pollfd.fd == fd under *UNIX > node->wevent == wevent under MSVC. > > I think we need a dedicated flag when we want to skip a given fd. Assume we revert this commit, the current logic looks good to me. We either get a 'fd' or a 'wevent' (can't get both). If we get a 'fd', we create a wevent and associate that 'fd' with 'wevent' via WSAEventSelect(). If we only get wevent, it will simply go to WaitForMultipleObjects(). > > Alin. > > >> -----Mesaj original----- >> De la: Gurucharan Shetty [mailto:[email protected]] >> Trimis: Wednesday, September 30, 2015 5:24 AM >> Către: Alin Serdean <[email protected]> >> Cc: Ben Pfaff <[email protected]>; Ilya Maximets <[email protected]>; >> dev <[email protected]>; Dyasly Sergey <[email protected]> >> Subiect: Re: [ovs-dev] [PATCH] poll-loop: fix assertion in poll_create_node >> >> I think Ben's patch would have fixed the problem had pollfd.fd actually held >> a >> negative value. Documentation in msdn says that it can, but when I step >> through the debugger, I see that a -1 set to that shows up us a large >> positive >> integer. For e.g., if I apply this hypothetical patch, I don't see test >> failures. >> >> diff --git a/lib/poll-loop.c b/lib/poll-loop.c index 36eb5ac..850eeac 100644 >> --- a/lib/poll-loop.c >> +++ b/lib/poll-loop.c >> @@ -297,7 +297,7 @@ free_poll_nodes(struct poll_loop *loop) >> HMAP_FOR_EACH_SAFE (node, next, hmap_node, &loop->poll_nodes) { >> hmap_remove(&loop->poll_nodes, &node->hmap_node); #ifdef >> _WIN32 >> - if (node->wevent && node->pollfd.fd) { >> + if (node->wevent && node->pollfd.fd <= 4400000) { >> WSAEventSelect(node->pollfd.fd, NULL, 0); >> CloseHandle(node->wevent); >> } >> @@ -341,7 +341,7 @@ poll_block(void) >> pollfds[i] = node->pollfd; >> #ifdef _WIN32 >> wevents[i] = node->wevent; >> - if (node->pollfd.fd && node->wevent) { >> + if (node->pollfd.fd <= 4400000 && node->wevent) { >> short int wsa_events = 0; >> if (node->pollfd.events & POLLIN) { >> wsa_events |= FD_READ | FD_ACCEPT | FD_CLOSE; >> >> >> I actually cannot understand what was the bug that created the original >> commit in question. I cannot think of a place in OVS where we send fd as >> zero to create_poll_node(). >> >> Ilya, >> Can you explain the original trigger that caused you to submit this patch? I >> would rather revert this unless I understand the reason. >> >> >> On Tue, Sep 29, 2015 at 5:07 PM, Gurucharan Shetty <[email protected]> >> wrote: >> > hash_2words takes unsigned int. I guess that is the problem? >> > >> > On Tue, Sep 29, 2015 at 5:00 PM, Alin Serdean >> > <[email protected]> wrote: >> >> I think so. I think for once we should do >> >> https://github.com/openvswitch/ovs/blob/master/lib/poll-loop.c#L119-L >> >> 123 before >> >> https://github.com/openvswitch/ovs/blob/master/lib/poll-loop.c#L115-L >> >> 116 >> >> >> >> I am compiling under debug and attaching a debugger to it. >> >> >> >> Alin. >> >> >> >>> -----Mesaj original----- >> >>> De la: Gurucharan Shetty [mailto:[email protected]] >> >>> Trimis: Wednesday, September 30, 2015 2:34 AM >> >>> Către: Alin Serdean <[email protected]> >> >>> Cc: Ben Pfaff <[email protected]>; Ilya Maximets >> >>> <[email protected]>; dev <[email protected]>; Dyasly >> Sergey >> >>> <[email protected]> >> >>> Subiect: Re: [ovs-dev] [PATCH] poll-loop: fix assertion in >> >>> poll_create_node >> >>> >> >>> Alin, >> >>> Reverting this commit on tip of master does not give me any unit >> >>> test failures. When you say there is more to it, do you mean that >> >>> this commit actually exposed some other bug? _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
