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

Message ID 20150929222541.GB16690@nicira.com
State Not Applicable
Headers show

Commit Message

Ben Pfaff Sept. 29, 2015, 10:25 p.m. UTC
Do we additionally need this?



On Tue, Sep 29, 2015 at 02:39:32PM -0700, Gurucharan Shetty wrote:
> Windows even handling selector functions is such that it does not like
> when fd is -1 (I have not root-caused the exact reason). This commit
> causes most of the unit tests on Windows to fail. So the larger
> question is, even though fd of zero is valid, do we really pass it to
> any of our poll node functions?
> 
> On Tue, Sep 22, 2015 at 9:34 AM, Ben Pfaff <blp@nicira.com> wrote:
> > Thanks, I understand now.  I applied this to master, changing Nikita's
> > Signed-off-by: to a Reported-by:.
> >
> > On Tue, Sep 22, 2015 at 07:30:47PM +0300, Ilya Maximets wrote:
> >> Author is me, Nikita found and analyzed the problem.
> >> You may change Signed-off-by to something more proper.
> >>
> >> Best regards, Ilya Maximets.
> >>
> >> On 22.09.2015 18:49, Ben Pfaff wrote:
> >> > On Tue, Sep 22, 2015 at 12:27:15PM +0300, Ilya Maximets wrote:
> >> >> Zero is a valid value for a file descriptor.
> >> >>
> >> >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> >> >> Signed-off-by: Nikita Kalyazin <n.kalyazin@samsung.com>
> >> >
> >> > Thanks for the patch!
> >> >
> >> > The sign-off chain is unclear.  Who is the author?  What did the other
> >> > person do?
> >> >
> >> > Thanks,
> >> >
> >> > Ben.
> >> >

Comments

Alin Serdean Sept. 29, 2015, 11:25 p.m. UTC | #1
I think there is much more behind the scenes.

Alin.

> -----Mesaj original-----

> De la: dev [mailto:dev-bounces@openvswitch.org] În numele Ben Pfaff

> Trimis: Wednesday, September 30, 2015 1:26 AM

> Către: Gurucharan Shetty <shettyg@nicira.com>

> Cc: 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

> 

> Do we additionally need this?

> 

> diff --git a/lib/poll-loop.c b/lib/poll-loop.c index 36eb5ac..28e98ad 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 >= 0) {

>              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 >= 0 && node->wevent) {

>              short int wsa_events = 0;

>              if (node->pollfd.events & POLLIN) {

>                  wsa_events |= FD_READ | FD_ACCEPT | FD_CLOSE;

> 

> 

> On Tue, Sep 29, 2015 at 02:39:32PM -0700, Gurucharan Shetty wrote:

> > Windows even handling selector functions is such that it does not like

> > when fd is -1 (I have not root-caused the exact reason). This commit

> > causes most of the unit tests on Windows to fail. So the larger

> > question is, even though fd of zero is valid, do we really pass it to

> > any of our poll node functions?

> >

> > On Tue, Sep 22, 2015 at 9:34 AM, Ben Pfaff <blp@nicira.com> wrote:

> > > Thanks, I understand now.  I applied this to master, changing

> > > Nikita's

> > > Signed-off-by: to a Reported-by:.

> > >

> > > On Tue, Sep 22, 2015 at 07:30:47PM +0300, Ilya Maximets wrote:

> > >> Author is me, Nikita found and analyzed the problem.

> > >> You may change Signed-off-by to something more proper.

> > >>

> > >> Best regards, Ilya Maximets.

> > >>

> > >> On 22.09.2015 18:49, Ben Pfaff wrote:

> > >> > On Tue, Sep 22, 2015 at 12:27:15PM +0300, Ilya Maximets wrote:

> > >> >> Zero is a valid value for a file descriptor.

> > >> >>

> > >> >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>

> > >> >> Signed-off-by: Nikita Kalyazin <n.kalyazin@samsung.com>

> > >> >

> > >> > Thanks for the patch!

> > >> >

> > >> > The sign-off chain is unclear.  Who is the author?  What did the

> > >> > other person do?

> > >> >

> > >> > Thanks,

> > >> >

> > >> > Ben.

> > >> >

> _______________________________________________

> dev mailing list

> dev@openvswitch.org

> http://openvswitch.org/mailman/listinfo/dev
Gurucharan Shetty Sept. 29, 2015, 11:33 p.m. UTC | #2
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?
Alin Serdean Sept. 30, 2015, midnight UTC | #3
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, 12:07 a.m. UTC | #4
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?
Ben Pfaff Sept. 30, 2015, 3:39 p.m. UTC | #5
Please don't drop the list.

On Wed, Sep 30, 2015 at 06:13:19PM +0300, Ilya Maximets wrote:
> On 30.09.2015 01:25, Ben Pfaff wrote:
> > Do we additionally need this?
> > 
> > diff --git a/lib/poll-loop.c b/lib/poll-loop.c
> > index 36eb5ac..28e98ad 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 >= 0) {
> >              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 >= 0 && node->wevent) {
> >              short int wsa_events = 0;
> >              if (node->pollfd.events & POLLIN) {
> >                  wsa_events |= FD_READ | FD_ACCEPT | FD_CLOSE;
> > 
> 
> Sorry, I missed this part in my patch. I think, this should be applied.

It was tested and did not fix the problem, because under Windows struct
pollfd's 'fd' member has an unsigned type.

I support reverting this patch.  It breaks a whole operating system
port.  That is much more important than avoiding a secondary problem due
to a buggy NIC driver.

Patch
diff mbox

diff --git a/lib/poll-loop.c b/lib/poll-loop.c
index 36eb5ac..28e98ad 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 >= 0) {
             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 >= 0 && node->wevent) {
             short int wsa_events = 0;
             if (node->pollfd.events & POLLIN) {
                 wsa_events |= FD_READ | FD_ACCEPT | FD_CLOSE;