On Fri, 11 Jul 2025 at 15:20, Fabiano Rosas <[email protected]> wrote:
>
> From: Peter Xu <[email protected]>
>
> Add the latency distribution too for blocktime, using order-of-two buckets.
> It accounts for all the faults, from either vCPU or non-vCPU threads. With
> prior rework, it's very easy to achieve by adding an array to account for
> faults in each buckets.
>
> Sample output for HMP (while for QMP it's simply an array):
>
> Postcopy Latency Distribution:
Hi; Coverity points out that there is a bug here (CID 1612248):
> +static const gchar *format_time_str(uint64_t us)
> +{
> + const char *units[] = {"us", "ms", "sec"};
> + int index = 0;
> +
> + while (us > 1000) {
> + us /= 1000;
> + if (++index >= (sizeof(units) - 1)) {
sizeof(units) is the size in bytes, which in this case is
24, as it is an array of three 8-byte pointers. So it's
not the right thing to use in bounds checking the array index.
You probably wanted ARRAY_SIZE(units). Also, the ++index
inside the comparison here seems unnecessarily confusing.
I would suggest something like
while (us > 1000 && index + 1 < ARRAY_SIZE(units)) {
us /= 1000;
index++;
}
which puts into the while condition the two conditions under
which we are OK to shift down a unit, and keeps it
clear that we maintain the invariant of "when we shift
down a unit we also divide the value by 1000".
> + break;
> + }
> + }
> +
> + return g_strdup_printf("%"PRIu64" %s", us, units[index]);
Otherwise this units[index] access could be off the end of
the array.
> +}
thanks
-- PMM
