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

Gurucharan Shetty shettyg at nicira.com
Mon Oct 5 21:01:26 UTC 2015


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



More information about the dev mailing list