[libvirt] [PATCHv3] util: make it more robust to calculate timeout value

[Martin Kletzander]

On Wed, May 27, 2015 at 04:58:22PM +0200, Martin Kletzander wrote:
>On Mon, May 25, 2015 at 02:22:42PM +0800, Wang Yufei wrote:
>>From: Zhang Bo <oscar.zhangbo at huawei.com>
>>
>>When we change system clock to years ago, a certain CPU may use up 100% cputime.
>>The reason is that in function virEventPollCalculateTimeout(), we assign the
>>unsigned long long result to an INT variable,
>>       *timeout = then - now; // timeout is INT, and then/now are long long
>>       if (*timeout < 0)
>>           *timeout = 0;
>>there's a chance that variable @then minus variable @now may be a very large number
>>that overflows INT value expression, then *timeout will be negative and be assigned to 0.
>>Next the 'poll' in function virEventPollRunOnce() will get into an 'endless' while loop there.
>>thus, the cpu that virEventPollRunOnce() thread runs on will go up to 100%.
>>
>>Although as we discussed before in https://www.redhat.com/archives/libvir-list/2015-May/msg00400.html
>>it should be prohibited to set-time while other applications are running, but it does
>>seems to have no harm to make the codes more robust.
>>
>>Signed-off-by: Wang Yufei <james.wangyufei at huawei.com>
>>Signed-off-by: Zhang Bo <oscar.zhangbo at huawei.com>
>>---
>>src/util/vireventpoll.c | 5 +++--
>>1 file changed, 3 insertions(+), 2 deletions(-)
>>
>>diff --git a/src/util/vireventpoll.c b/src/util/vireventpoll.c
>>index ffda206..4e4f407 100644
>>--- a/src/util/vireventpoll.c
>>+++ b/src/util/vireventpoll.c
>>@@ -357,9 +357,10 @@ static int virEventPollCalculateTimeout(int *timeout)
>>            return -1;
>>
>>        EVENT_DEBUG("Schedule timeout then=%llu now=%llu", then, now);
>>-        *timeout = then - now;
>>-        if (*timeout < 0)
>>+        if (then <= now)
>>            *timeout = 0;
>>+        else
>>+            *timeout = ((then-now) > INT_MAX) ? INT_MAX : (then-now);
>

I added spaces around the minus sign.

>This will work, although the following would be nicer to read since
>there were such problems with the calculation in earlier versions:
>
> if (then <= now) {
>     *timeout = 0;
> } else {
>     long long tmp = then - now;

And of course this would be unsigned.

>     if (tmp > INT_MAX)
>         *timeout = INT_MAX
>     else
>         *timeout = tmp;
> }
>
>But that, of course, has the same meaning as your version, which is
>fine.  ACK to that, I'll push it in a while.
>
>>    } else {
>>        *timeout = -1;
>>    }
>>--
>>1.7.12.4
>>
>>
>>--
>>libvir-list mailing list
>>libvir-list at redhat.com
>>https://www.redhat.com/mailman/listinfo/libvir-list



>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150527/77a68df5/attachment-0001.sig>