[ovs-dev] poll-loop: Fix a bug while finding a poll node.
diff mbox

Message ID 1443647927-29701-1-git-send-email-gshetty@nicira.com
State Accepted
Headers show

Commit Message

Gurucharan Shetty Sept. 30, 2015, 9:18 p.m. UTC
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(-)

Comments

Alin Serdean Sept. 30, 2015, 9:29 p.m. UTC | #1
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
Gurucharan Shetty Sept. 30, 2015, 9:36 p.m. UTC | #2
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
>
Ben Pfaff Oct. 5, 2015, 7:58 p.m. UTC | #3
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;
>          }
>      }
Gurucharan Shetty Oct. 5, 2015, 9:01 p.m. UTC | #4
>
> 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;
>>          }
>>      }
Ben Pfaff Oct. 5, 2015, 10:18 p.m. UTC | #5
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>

Patch
diff mbox

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;
         }
     }