| Message ID | 1443647927-29701-1-git-send-email-gshetty@nicira.com |
|---|---|
| State | Accepted |
| Headers | show |
I think it would be nice to have ovs_assert(!fd != !wevent); ifdefed under Windows. And on the rest of the platforms: ovs_assert(!fd != !wevent); And probably also for poll_create_node as well. Like Ilya and Nikita were mentioning. Otherwise: Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com> > -----Mesaj original----- > De la: Gurucharan Shetty [mailto:shettyg@nicira.com] > Trimis: Thursday, October 1, 2015 12:19 AM > Către: dev@openvswitch.org > Cc: Alin Serdean <aserdean@cloudbasesolutions.com>; Gurucharan Shetty > <gshetty@nicira.com> > Subiect: [PATCH] poll-loop: Fix a bug while finding a poll node. > > When a poll_node is created, it gets either a 'fd' or a 'wevent' (can't get > both). When the poll_node is searched for previous creations on that 'fd' or > 'wevent', the search criteria was wrong for Windows. In Windows, when a > 'fd' is received in poll_create_node, we create a corresponding 'wevent'. So > while searching for that 'fd', we should not look for 'wevent' in the > hmap_node. > > Reported-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com> > Signed-off-by: Gurucharan Shetty <gshetty@nicira.com> > --- > lib/poll-loop.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/lib/poll-loop.c b/lib/poll-loop.c index 3c4b55c..60e1f6e 100644 > --- a/lib/poll-loop.c > +++ b/lib/poll-loop.c > @@ -57,16 +57,20 @@ struct poll_loop { > > static struct poll_loop *poll_loop(void); > > -/* Look up the node with same fd and wevent. */ > +/* Look up the node with same fd or wevent. */ > static struct poll_node * > find_poll_node(struct poll_loop *loop, int fd, HANDLE wevent) { > struct poll_node *node; > > + /* Both 'fd' and 'wevent' cannot be set. */ > + ovs_assert(!fd != !wevent); > + > HMAP_FOR_EACH_WITH_HASH (node, hmap_node, > hash_2words(fd, (uint32_t)wevent), > &loop->poll_nodes) { > - if (node->pollfd.fd == fd && node->wevent == wevent) { > + if ((fd && node->pollfd.fd == fd) > + || (wevent && node->wevent == wevent)) { > return node; > } > } > -- > 1.7.9.5
On Wed, Sep 30, 2015 at 2:29 PM, Alin Serdean <aserdean@cloudbasesolutions.com> wrote: > I think it would be nice to have > ovs_assert(!fd != !wevent); > ifdefed under Windows. > And on the rest of the platforms: > ovs_assert(!fd != !wevent); I did not follow what you meant above at all. Can you elaborate? > > And probably also for poll_create_node as well. > > Like Ilya and Nikita were mentioning. > > Otherwise: > Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com> > > >> -----Mesaj original----- >> De la: Gurucharan Shetty [mailto:shettyg@nicira.com] >> Trimis: Thursday, October 1, 2015 12:19 AM >> Către: dev@openvswitch.org >> Cc: Alin Serdean <aserdean@cloudbasesolutions.com>; Gurucharan Shetty >> <gshetty@nicira.com> >> Subiect: [PATCH] poll-loop: Fix a bug while finding a poll node. >> >> When a poll_node is created, it gets either a 'fd' or a 'wevent' (can't get >> both). When the poll_node is searched for previous creations on that 'fd' or >> 'wevent', the search criteria was wrong for Windows. In Windows, when a >> 'fd' is received in poll_create_node, we create a corresponding 'wevent'. So >> while searching for that 'fd', we should not look for 'wevent' in the >> hmap_node. >> >> Reported-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com> >> Signed-off-by: Gurucharan Shetty <gshetty@nicira.com> >> --- >> lib/poll-loop.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/lib/poll-loop.c b/lib/poll-loop.c index 3c4b55c..60e1f6e 100644 >> --- a/lib/poll-loop.c >> +++ b/lib/poll-loop.c >> @@ -57,16 +57,20 @@ struct poll_loop { >> >> static struct poll_loop *poll_loop(void); >> >> -/* Look up the node with same fd and wevent. */ >> +/* Look up the node with same fd or wevent. */ >> static struct poll_node * >> find_poll_node(struct poll_loop *loop, int fd, HANDLE wevent) { >> struct poll_node *node; >> >> + /* Both 'fd' and 'wevent' cannot be set. */ >> + ovs_assert(!fd != !wevent); >> + >> HMAP_FOR_EACH_WITH_HASH (node, hmap_node, >> hash_2words(fd, (uint32_t)wevent), >> &loop->poll_nodes) { >> - if (node->pollfd.fd == fd && node->wevent == wevent) { >> + if ((fd && node->pollfd.fd == fd) >> + || (wevent && node->wevent == wevent)) { >> return node; >> } >> } >> -- >> 1.7.9.5 >
On Wed, Sep 30, 2015 at 02:18:47PM -0700, Gurucharan Shetty wrote: > When a poll_node is created, it gets either a 'fd' or > a 'wevent' (can't get both). When the poll_node is > searched for previous creations on that 'fd' or 'wevent', > the search criteria was wrong for Windows. In Windows, > when a 'fd' is received in poll_create_node, we create a > corresponding 'wevent'. So while searching for that 'fd', > we should not look for 'wevent' in the hmap_node. > > Reported-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com> > Signed-off-by: Gurucharan Shetty <gshetty@nicira.com> I think that when the assertion is true (presumably always), the new and the old 'if' conditions are equivalent. Is that right? > --- > lib/poll-loop.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/lib/poll-loop.c b/lib/poll-loop.c > index 3c4b55c..60e1f6e 100644 > --- a/lib/poll-loop.c > +++ b/lib/poll-loop.c > @@ -57,16 +57,20 @@ struct poll_loop { > > static struct poll_loop *poll_loop(void); > > -/* Look up the node with same fd and wevent. */ > +/* Look up the node with same fd or wevent. */ > static struct poll_node * > find_poll_node(struct poll_loop *loop, int fd, HANDLE wevent) > { > struct poll_node *node; > > + /* Both 'fd' and 'wevent' cannot be set. */ > + ovs_assert(!fd != !wevent); > + > HMAP_FOR_EACH_WITH_HASH (node, hmap_node, > hash_2words(fd, (uint32_t)wevent), > &loop->poll_nodes) { > - if (node->pollfd.fd == fd && node->wevent == wevent) { > + if ((fd && node->pollfd.fd == fd) > + || (wevent && node->wevent == wevent)) { > return node; > } > }
> > I think that when the assertion is true (presumably always), the new and > the old 'if' conditions are equivalent. Is that right? They are not equivalent. In poll_create_node, we have the following piece of code. /* Check for duplicate. If found, "or" the events. */ node = find_poll_node(loop, fd, wevent); if (node) { node->pollfd.events |= events; } else { node = xzalloc(sizeof *node); hmap_insert(&loop->poll_nodes, &node->hmap_node, hash_2words(fd, (uint32_t)wevent)); node->pollfd.fd = fd; node->pollfd.events = events; #ifdef _WIN32 if (!wevent) { wevent = CreateEvent(NULL, FALSE, FALSE, NULL); } #endif node->wevent = wevent; node->where = where; } For Windows, when 'fd' is set, when find_poll_node() returns false, we create a new poll_node whose key is 'fd' (wevent is zero). We later set node->wevent to the return value of CreateEvent(). When find_poll_node() is called again with the same 'fd', the current if condition in poll_fd_node() is wrong, because it does: "node->pollfd.fd == fd && node->wevent == wevent". The condition will never be true because 'wevent' passed to find_poll_node is 0 whereas node->wevent has a value set. Did my above explanation make sense? > >> --- >> lib/poll-loop.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/lib/poll-loop.c b/lib/poll-loop.c >> index 3c4b55c..60e1f6e 100644 >> --- a/lib/poll-loop.c >> +++ b/lib/poll-loop.c >> @@ -57,16 +57,20 @@ struct poll_loop { >> >> static struct poll_loop *poll_loop(void); >> >> -/* Look up the node with same fd and wevent. */ >> +/* Look up the node with same fd or wevent. */ >> static struct poll_node * >> find_poll_node(struct poll_loop *loop, int fd, HANDLE wevent) >> { >> struct poll_node *node; >> >> + /* Both 'fd' and 'wevent' cannot be set. */ >> + ovs_assert(!fd != !wevent); >> + >> HMAP_FOR_EACH_WITH_HASH (node, hmap_node, >> hash_2words(fd, (uint32_t)wevent), >> &loop->poll_nodes) { >> - if (node->pollfd.fd == fd && node->wevent == wevent) { >> + if ((fd && node->pollfd.fd == fd) >> + || (wevent && node->wevent == wevent)) { >> return node; >> } >> }
On Mon, Oct 05, 2015 at 02:01:26PM -0700, Gurucharan Shetty wrote: > > > > I think that when the assertion is true (presumably always), the new and > > the old 'if' conditions are equivalent. Is that right? > > They are not equivalent. In poll_create_node, we have the following > piece of code. > > /* Check for duplicate. If found, "or" the events. */ > node = find_poll_node(loop, fd, wevent); > if (node) { > node->pollfd.events |= events; > } else { > node = xzalloc(sizeof *node); > hmap_insert(&loop->poll_nodes, &node->hmap_node, > hash_2words(fd, (uint32_t)wevent)); > node->pollfd.fd = fd; > node->pollfd.events = events; > #ifdef _WIN32 > if (!wevent) { > wevent = CreateEvent(NULL, FALSE, FALSE, NULL); > } > #endif > node->wevent = wevent; > node->where = where; > } > > For Windows, when 'fd' is set, when find_poll_node() returns false, we > create a new poll_node whose key is 'fd' (wevent is zero). We later > set node->wevent to the return value of CreateEvent(). > > When find_poll_node() is called again with the same 'fd', the current > if condition in poll_fd_node() is wrong, because it does: > "node->pollfd.fd == fd && node->wevent == wevent". The condition will > never be true because 'wevent' passed to find_poll_node is 0 whereas > node->wevent has a value set. > > Did my above explanation make sense? Yes. Acked-by: Ben Pfaff <blp@nicira.com>
diff --git a/lib/poll-loop.c b/lib/poll-loop.c index 3c4b55c..60e1f6e 100644 --- a/lib/poll-loop.c +++ b/lib/poll-loop.c @@ -57,16 +57,20 @@ struct poll_loop { static struct poll_loop *poll_loop(void); -/* Look up the node with same fd and wevent. */ +/* Look up the node with same fd or wevent. */ static struct poll_node * find_poll_node(struct poll_loop *loop, int fd, HANDLE wevent) { struct poll_node *node; + /* Both 'fd' and 'wevent' cannot be set. */ + ovs_assert(!fd != !wevent); + HMAP_FOR_EACH_WITH_HASH (node, hmap_node, hash_2words(fd, (uint32_t)wevent), &loop->poll_nodes) { - if (node->pollfd.fd == fd && node->wevent == wevent) { + if ((fd && node->pollfd.fd == fd) + || (wevent && node->wevent == wevent)) { return node; } }
When a poll_node is created, it gets either a 'fd' or a 'wevent' (can't get both). When the poll_node is searched for previous creations on that 'fd' or 'wevent', the search criteria was wrong for Windows. In Windows, when a 'fd' is received in poll_create_node, we create a corresponding 'wevent'. So while searching for that 'fd', we should not look for 'wevent' in the hmap_node. Reported-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com> Signed-off-by: Gurucharan Shetty <gshetty@nicira.com> --- lib/poll-loop.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)