|
From: | Philippe Mathieu-Daudé |
Subject: | Re: [Qemu-block] [Qemu-devel] [PATCH] throttle: Assert that bkt->max is valid in throttle_compute_wait() |
Date: | Wed, 13 Sep 2017 13:31:58 -0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
Hi Alberto, On 09/13/2017 05:28 AM, Alberto Garcia wrote:
If bkt->max == 0 and bkt->burst_length > 1 then we could have a division by 0 in throttle_do_compute_wait(). That configuration is however not permitted and is already detected by throttle_is_valid(), but let's assert it in throttle_compute_wait() to make it explicit.
This is correct but I'm not sure this is enough, as throttle_compute_wait() is exported/public, however it seems testing is the only reason to export it.
Also I spent 10min looking at it thinking about how bkt->max is used, before to realize there should be a simpler way to write this (KISS).
Anyway I'm queuing your patch in my /static-analysis branch I plan to this week where I use a self-explanatory macro instead of assert() and will see if I can simplify it (note that I'm not a maintainer!).
Regards, Phil.
Found by Coverity (CID: 1381016). Signed-off-by: Alberto Garcia <address@hidden> --- util/throttle.c | 1 + 1 file changed, 1 insertion(+) diff --git a/util/throttle.c b/util/throttle.c index 06bf916adc..b38e742da5 100644 --- a/util/throttle.c +++ b/util/throttle.c @@ -124,6 +124,7 @@ int64_t throttle_compute_wait(LeakyBucket *bkt) /* If the main bucket is not full yet we still have to check the * burst bucket in order to enforce the burst limit */ if (bkt->burst_length > 1) { + assert(bkt->max > 0); /* see throttle_is_valid() */ extra = bkt->burst_level - burst_bucket_size; if (extra > 0) { return throttle_do_compute_wait(bkt->max, extra);
[Prev in Thread] | Current Thread | [Next in Thread] |