| Message ID | 20150929222541.GB16690@nicira.com |
|---|---|
| State | Not Applicable |
| Headers | show |
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
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?
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?
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?
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.
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;