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

Message ID CAHbON5rYxXnTN0rkvnOjxLXE-dhRJXa0k_i66rpYW+624ifhBg@mail.gmail.com
State Not Applicable
Headers show

Commit Message

Gurucharan Shetty Sept. 30, 2015, 2:23 a.m. UTC
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.



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@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@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-L123 before https://github.com/openvswitch/ovs/blob/master/lib/poll-loop.c#L115-L116
>>
>> I am compiling under debug and attaching a debugger to it.
>>
>> Alin.
>>
>>> -----Mesaj original-----
>>> De la: Gurucharan Shetty [mailto:shettyg@nicira.com]
>>> Trimis: Wednesday, September 30, 2015 2:34 AM
>>> Către: Alin Serdean <aserdean@cloudbasesolutions.com>
>>> Cc: Ben Pfaff <blp@nicira.com>; Ilya Maximets <i.maximets@samsung.com>;
>>> dev <dev@openvswitch.org>; Dyasly Sergey <s.dyasly@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?

Comments

Alin Serdean Sept. 30, 2015, 2:44 a.m. UTC | #1
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@nicira.com]

> Trimis: Wednesday, September 30, 2015 5:24 AM

> Către: Alin Serdean <aserdean@cloudbasesolutions.com>

> Cc: Ben Pfaff <blp@nicira.com>; Ilya Maximets <i.maximets@samsung.com>;

> dev <dev@openvswitch.org>; Dyasly Sergey <s.dyasly@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@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@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@nicira.com]

> >>> Trimis: Wednesday, September 30, 2015 2:34 AM

> >>> Către: Alin Serdean <aserdean@cloudbasesolutions.com>

> >>> Cc: Ben Pfaff <blp@nicira.com>; Ilya Maximets

> >>> <i.maximets@samsung.com>; dev <dev@openvswitch.org>; Dyasly

> Sergey

> >>> <s.dyasly@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?
Gurucharan Shetty Sept. 30, 2015, 3:32 a.m. UTC | #2
On Tue, Sep 29, 2015 at 7:44 PM, Alin Serdean
<aserdean@cloudbasesolutions.com> 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:shettyg@nicira.com]
>> Trimis: Wednesday, September 30, 2015 5:24 AM
>> Către: Alin Serdean <aserdean@cloudbasesolutions.com>
>> Cc: Ben Pfaff <blp@nicira.com>; Ilya Maximets <i.maximets@samsung.com>;
>> dev <dev@openvswitch.org>; Dyasly Sergey <s.dyasly@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@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@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@nicira.com]
>> >>> Trimis: Wednesday, September 30, 2015 2:34 AM
>> >>> Către: Alin Serdean <aserdean@cloudbasesolutions.com>
>> >>> Cc: Ben Pfaff <blp@nicira.com>; Ilya Maximets
>> >>> <i.maximets@samsung.com>; dev <dev@openvswitch.org>; Dyasly
>> Sergey
>> >>> <s.dyasly@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?
Gurucharan Shetty Sept. 30, 2015, 3:44 a.m. UTC | #3
>
> 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().
I see now what you mean about a problem in find_poll_node(). I think
separating it out like you mention won't work though. We need to check
if 'fd' is specified, in which case we 'if' on 'fd'. If wevent is
specified, 'if' on 'wevent'.

>
>
>
>>
>> Alin.
>>
>>
>>> -----Mesaj original-----
>>> De la: Gurucharan Shetty [mailto:shettyg@nicira.com]
>>> Trimis: Wednesday, September 30, 2015 5:24 AM
>>> Către: Alin Serdean <aserdean@cloudbasesolutions.com>
>>> Cc: Ben Pfaff <blp@nicira.com>; Ilya Maximets <i.maximets@samsung.com>;
>>> dev <dev@openvswitch.org>; Dyasly Sergey <s.dyasly@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@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@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@nicira.com]
>>> >>> Trimis: Wednesday, September 30, 2015 2:34 AM
>>> >>> Către: Alin Serdean <aserdean@cloudbasesolutions.com>
>>> >>> Cc: Ben Pfaff <blp@nicira.com>; Ilya Maximets
>>> >>> <i.maximets@samsung.com>; dev <dev@openvswitch.org>; Dyasly
>>> Sergey
>>> >>> <s.dyasly@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?
Ben Pfaff Sept. 30, 2015, 3:49 a.m. UTC | #4
On Wed, Sep 30, 2015 at 02:44:15AM +0000, Alin Serdean wrote:
> 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.

That pages talks, twice, about negative values of fd.  That's
*intentionally* goddamn deceptive for talking about an unsigned type.
Microsoft should fix their documentation.  Maybe someone with an account
should add a comment on the page.

But: maybe we're doing totally the wrong thing if a SOCKET is really a
UINT_PTR, because that means we need a 64-bit (unsigned) type and we're
using an int in the function prototypes.  Maybe we need to change all
the function prototypes to use `int' on Unix and `SOCKET' on Windows
(presumably via typedef).

On the other hand, I guess socket() can't just be returning random
64-bit numbers as SOCKET values, since that would totally break the
select() function that Winsock appears to support at least somewhat.
Maybe it always returns a small integer.  (However, if it ever returns
zero...).
Gurucharan Shetty Sept. 30, 2015, 2:49 p.m. UTC | #5
On Tue, Sep 29, 2015 at 8:49 PM, Ben Pfaff <blp@nicira.com> wrote:
> On Wed, Sep 30, 2015 at 02:44:15AM +0000, Alin Serdean wrote:
>> 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.
>
> That pages talks, twice, about negative values of fd.  That's
> *intentionally* goddamn deceptive for talking about an unsigned type.
> Microsoft should fix their documentation.  Maybe someone with an account
> should add a comment on the page.
>
> But: maybe we're doing totally the wrong thing if a SOCKET is really a
> UINT_PTR, because that means we need a 64-bit (unsigned) type and we're
> using an int in the function prototypes.  Maybe we need to change all
> the function prototypes to use `int' on Unix and `SOCKET' on Windows
> (presumably via typedef).

Googling a bit around, it looks like many people have made the
assumption that even though on 64 bit Windows, socket can
theoretically return 64 bit number, it actually does not. The
following comment from OpenSSL (here:
https://github.com/openssl/openssl/blob/master/e_os.h#L478) is cited
as a justification :

/*
* Even though sizeof(SOCKET) is 8, it's safe to cast it to int, because
* the value constitutes an index in per-process table of limited size
* and not a real pointer.
*/






>
> On the other hand, I guess socket() can't just be returning random
> 64-bit numbers as SOCKET values, since that would totally break the
> select() function that Winsock appears to support at least somewhat.
> Maybe it always returns a small integer.  (However, if it ever returns
> zero...).
Ilya Maximets Sept. 30, 2015, 3:10 p.m. UTC | #6
On 30.09.2015 05:23, Gurucharan Shetty wrote:
> 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.

In my case fd 0 was unexpectedly closed and tap interface opened on this fd.
So, this assert was triggered. It is not a common case, I agree, but it is not
right anyway to expect, that we will never get fd 0 as a result of open().
If you want to revert this patch you should forbid opening fd 0 at least
inside netdev-linux.

Best regards, Ilya Maximets.

> 
> 
> On Tue, Sep 29, 2015 at 5:07 PM, Gurucharan Shetty <shettyg@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@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-L123 before https://github.com/openvswitch/ovs/blob/master/lib/poll-loop.c#L115-L116
>>>
>>> I am compiling under debug and attaching a debugger to it.
>>>
>>> Alin.
>>>
>>>> -----Mesaj original-----
>>>> De la: Gurucharan Shetty [mailto:shettyg@nicira.com]
>>>> Trimis: Wednesday, September 30, 2015 2:34 AM
>>>> Către: Alin Serdean <aserdean@cloudbasesolutions.com>
>>>> Cc: Ben Pfaff <blp@nicira.com>; Ilya Maximets <i.maximets@samsung.com>;
>>>> dev <dev@openvswitch.org>; Dyasly Sergey <s.dyasly@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?
>
Gurucharan Shetty Sept. 30, 2015, 3:17 p.m. UTC | #7
>
> In my case fd 0 was unexpectedly closed and tap interface opened on this fd.
Can you elaborate?


> So, this assert was triggered. It is not a common case, I agree, but it is not
> right anyway to expect, that we will never get fd 0 as a result of open().
> If you want to revert this patch you should forbid opening fd 0 at least
> inside netdev-linux.
>
> Best regards, Ilya Maximets.
>
>>
>>
>> On Tue, Sep 29, 2015 at 5:07 PM, Gurucharan Shetty <shettyg@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@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-L123 before https://github.com/openvswitch/ovs/blob/master/lib/poll-loop.c#L115-L116
>>>>
>>>> I am compiling under debug and attaching a debugger to it.
>>>>
>>>> Alin.
>>>>
>>>>> -----Mesaj original-----
>>>>> De la: Gurucharan Shetty [mailto:shettyg@nicira.com]
>>>>> Trimis: Wednesday, September 30, 2015 2:34 AM
>>>>> Către: Alin Serdean <aserdean@cloudbasesolutions.com>
>>>>> Cc: Ben Pfaff <blp@nicira.com>; Ilya Maximets <i.maximets@samsung.com>;
>>>>> dev <dev@openvswitch.org>; Dyasly Sergey <s.dyasly@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?
>>
Ilya Maximets Sept. 30, 2015, 3:31 p.m. UTC | #8
On 30.09.2015 18:17, Gurucharan Shetty wrote:
>>
>> In my case fd 0 was unexpectedly closed and tap interface opened on this fd.
> Can you elaborate?

fd 0 was closed due to bug inside the driver of our NIC. After that it was opened
for internal port (tap).

> 
> 
>> So, this assert was triggered. It is not a common case, I agree, but it is not
>> right anyway to expect, that we will never get fd 0 as a result of open().
>> If you want to revert this patch you should forbid opening fd 0 at least
>> inside netdev-linux.
>>
>> Best regards, Ilya Maximets.
>>
>>>
>>>
>>> On Tue, Sep 29, 2015 at 5:07 PM, Gurucharan Shetty <shettyg@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@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-L123 before https://github.com/openvswitch/ovs/blob/master/lib/poll-loop.c#L115-L116
>>>>>
>>>>> I am compiling under debug and attaching a debugger to it.
>>>>>
>>>>> Alin.
>>>>>
>>>>>> -----Mesaj original-----
>>>>>> De la: Gurucharan Shetty [mailto:shettyg@nicira.com]
>>>>>> Trimis: Wednesday, September 30, 2015 2:34 AM
>>>>>> Către: Alin Serdean <aserdean@cloudbasesolutions.com>
>>>>>> Cc: Ben Pfaff <blp@nicira.com>; Ilya Maximets <i.maximets@samsung.com>;
>>>>>> dev <dev@openvswitch.org>; Dyasly Sergey <s.dyasly@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?
>>>
>

Patch
diff mbox

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;