Coverity has caught a bug in the formatting of time intervals for
postcopy latency distribution display in 'info migrate'.
While bounds checking the labels array, sizeof is incorrectly being
used. ARRAY_SIZE is the correct form of obtaining the size of an
array.
Fixes: 3345fb3b6d ("migration/postcopy: Add latency distribution report for blocktime")
Resolves: Coverity CID 1612248
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
migration/migration-hmp-cmds.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index cef5608210..bb954881d7 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -57,11 +57,9 @@ static const gchar *format_time_str(uint64_t us)
const char *units[] = {"us", "ms", "sec"};
int index = 0;
- while (us > 1000) {
+ while (us > 1000 && index + 1 < ARRAY_SIZE(units)) {
us /= 1000;
- if (++index >= (sizeof(units) - 1)) {
- break;
- }
+ index++;
}
return g_strdup_printf("%"PRIu64" %s", us, units[index]);
--
2.35.3On Tue, Jul 15, 2025 at 09:45:51AM -0300, Fabiano Rosas wrote:
> Coverity has caught a bug in the formatting of time intervals for
> postcopy latency distribution display in 'info migrate'.
>
> While bounds checking the labels array, sizeof is incorrectly being
> used. ARRAY_SIZE is the correct form of obtaining the size of an
> array.
>
> Fixes: 3345fb3b6d ("migration/postcopy: Add latency distribution report for blocktime")
> Resolves: Coverity CID 1612248
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> migration/migration-hmp-cmds.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index cef5608210..bb954881d7 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -57,11 +57,9 @@ static const gchar *format_time_str(uint64_t us)
> const char *units[] = {"us", "ms", "sec"};
> int index = 0;
>
> - while (us > 1000) {
> + while (us > 1000 && index + 1 < ARRAY_SIZE(units)) {
> us /= 1000;
> - if (++index >= (sizeof(units) - 1)) {
> - break;
> - }
> + index++;
> }
>
> return g_strdup_printf("%"PRIu64" %s", us, units[index]);
It occurrs to me that this is the same basic algorithmic problem as
converting storage sizes from bytes to KB/MB/GB/etc.
We have size_to_str() which does this conversion without needing any
loop at all.
Then there is freq_to_str() which has similar purpose but still uses
a loop, instead of the shortcut size_to_str has.
IMHO we should have a 'scaled_int_to_str' method that is common to
any place we need such scaled integer string conversions, and then
wrappers like "size_to_str", "freq_to_str" and "duration_to_str"
that pass in the required list of unit strings, and the divisor
and requested decimal precision, etc.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, 15 Jul 2025 at 18:49, Fabiano Rosas <farosas@suse.de> wrote:
> @@ -57,11 +57,9 @@ static const gchar *format_time_str(uint64_t us)
> const char *units[] = {"us", "ms", "sec"};
> int index = 0;
>
> - while (us > 1000) {
> + while (us > 1000 && index + 1 < ARRAY_SIZE(units)) {
> us /= 1000;
> - if (++index >= (sizeof(units) - 1)) {
> - break;
> - }
> + index++;
> }
>
> return g_strdup_printf("%"PRIu64" %s", us, units[index]);
* This loop is rather confusing.
* Is the while loop converting microseconds (us) to seconds with: us
/= 1000 ? ie. index shall mostly be 2 = "sec", except for the range =
1000000 - 1000999, when us / 1000 => 1000 would break the while loop
and it'd return string "1000 ms".
===
#define MS (1000)
#define US (MS * 1000)
#define NS (US * 1000)
if (n >= NS)
n /= NS;
else if (n >= US)
n /= US;
else if (n >= MS)
n /= MS;
return g_strdup_printf("%"PRIu64" sec", n);
===
* Does the above simplification look right? It shall always return
seconds as: "<n> sec"
Thank you.
---
- Prasad
Prasad Pandit <ppandit@redhat.com> writes:
> On Tue, 15 Jul 2025 at 18:49, Fabiano Rosas <farosas@suse.de> wrote:
>> @@ -57,11 +57,9 @@ static const gchar *format_time_str(uint64_t us)
>> const char *units[] = {"us", "ms", "sec"};
>> int index = 0;
>>
>> - while (us > 1000) {
>> + while (us > 1000 && index + 1 < ARRAY_SIZE(units)) {
>> us /= 1000;
>> - if (++index >= (sizeof(units) - 1)) {
>> - break;
>> - }
>> + index++;
>> }
>>
>> return g_strdup_printf("%"PRIu64" %s", us, units[index]);
>
> * This loop is rather confusing.
>
> * Is the while loop converting microseconds (us) to seconds with: us
> /= 1000 ? ie. index shall mostly be 2 = "sec", except for the range =
> 1000000 - 1000999, when us / 1000 => 1000 would break the while loop
> and it'd return string "1000 ms".
Good catch. The condition should be >=.
> ===
> #define MS (1000)
> #define US (MS * 1000)
> #define NS (US * 1000)
>
> if (n >= NS)
> n /= NS;
> else if (n >= US)
> n /= US;
> else if (n >= MS)
> n /= MS;
>
> return g_strdup_printf("%"PRIu64" sec", n);
> ===
>
> * Does the above simplification look right? It shall always return
> seconds as: "<n> sec"
>
But then that's "0 sec" for 1000000 us.
>
> Thank you.
> ---
> - Prasad
On Wed, 16 Jul 2025 at 19:06, Fabiano Rosas <farosas@suse.de> wrote: > The condition should be >=. * I'm thinking of doing away with the loop and the string array. The array has 3 values of which only one gets used due to conversion to seconds. > But then that's "0 sec" for 1000000 us. #define US (MS * 1000) => 1000000 When us = 1000000, us / US should return "1 sec", no? Thank you. --- - Prasad
Prasad Pandit <ppandit@redhat.com> writes: > On Wed, 16 Jul 2025 at 19:06, Fabiano Rosas <farosas@suse.de> wrote: >> The condition should be >=. > > * I'm thinking of doing away with the loop and the string array. The > array has 3 values of which only one gets used due to conversion to > seconds. The point is not to convert to seconds, but to fit the number of microseconds in a range. Take a look at the output of 'info migrate -a' with postcopy-blocktime capability enabled. > >> But then that's "0 sec" for 1000000 us. > > #define US (MS * 1000) => 1000000 > > When us = 1000000, us / US should return "1 sec", no? > Sorry, I meant when the number of microseconds is smaller than a second. > Thank you. > --- > - Prasad
On 15/7/25 14:45, Fabiano Rosas wrote:
> Coverity has caught a bug in the formatting of time intervals for
> postcopy latency distribution display in 'info migrate'.
>
> While bounds checking the labels array, sizeof is incorrectly being
> used. ARRAY_SIZE is the correct form of obtaining the size of an
> array.
>
> Fixes: 3345fb3b6d ("migration/postcopy: Add latency distribution report for blocktime")
> Resolves: Coverity CID 1612248
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> migration/migration-hmp-cmds.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
© 2016 - 2025 Red Hat, Inc.