[ovs-dev] [PATCH] poll-loop: fix assertion in poll_create_node

[Alin Serdean]

The ides is the following:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms740094%28v=vs.85%29.aspx
and SOCKET is defined as:
typedef UINT_PTR        SOCKET;

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.

Alin.


> -----Mesaj original-----
> De la: Gurucharan Shetty [mailto:shettyg at nicira.com]
> Trimis: Wednesday, September 30, 2015 5:24 AM
> Către: Alin Serdean <aserdean at cloudbasesolutions.com>
> Cc: Ben Pfaff <blp at nicira.com>; Ilya Maximets <i.maximets at samsung.com>;
> dev <dev at openvswitch.org>; Dyasly Sergey <s.dyasly at samsung.com>
> 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 <shettyg at nicira.com>
> wrote:
> > hash_2words takes unsigned int. I guess that is the problem?
> >
> > On Tue, Sep 29, 2015 at 5:00 PM, Alin Serdean
> > <aserdean at cloudbasesolutions.com> 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:shettyg at nicira.com]
> >>> Trimis: Wednesday, September 30, 2015 2:34 AM
> >>> Către: Alin Serdean <aserdean at cloudbasesolutions.com>
> >>> Cc: Ben Pfaff <blp at nicira.com>; Ilya Maximets
> >>> <i.maximets at samsung.com>; dev <dev at openvswitch.org>; Dyasly
> Sergey
> >>> <s.dyasly at samsung.com>
> >>> 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?