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

On Mon, May 25, 2015 at 02:22:42PM +0800, Wang Yufei wrote: >From: Zhang Bo <oscar.zhangbo(a)huawei.com&gt; > >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(a)huawei.com&gt; >Signed-off-by: Zhang Bo <oscar.zhangbo(a)huawei.com&gt; >--- >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);

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;

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 &gt;libvir-list(a)redhat.com >https://www.redhat.com/mailman/listinfo/libvir-list

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list