On Fri, 11 Jul 2025 at 15:20, Fabiano Rosas <faro...@suse.de> wrote:
>
> From: Peter Xu <pet...@redhat.com>
>
> 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

Reply via email to