| Message ID | 20191122080039.12771-2-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix incorrect integer->float conversion caught by clang | expand |
On 11/22/19 9:00 AM, Markus Armbruster wrote: > From: Fangrui Song <i@maskray.me> > > Clang does not like qmp_migrate_set_downtime()'s code to clamp double > @value to 0..INT64_MAX: > > qemu/migration/migration.c:2038:24: error: implicit conversion from 'long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Werror,-Wimplicit-int-float-conversion] > > The warning will be enabled by default in clang 10. It is not > available for clang <= 9. > > The clamp is actually useless; @value is checked to be within > 0..MAX_MIGRATE_DOWNTIME_SECONDS immediately before. Delete it. > > While there, make the conversion from double to int64_t explicit. > > Signed-off-by: Fangrui Song <i@maskray.me> > Reviewed-by: Markus Armbruster <armbru@redhat.com> > [Patch split, commit message improved] > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > migration/migration.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Markus Armbruster <armbru@redhat.com> wrote: > From: Fangrui Song <i@maskray.me> > > Clang does not like qmp_migrate_set_downtime()'s code to clamp double > @value to 0..INT64_MAX: > > qemu/migration/migration.c:2038:24: error: implicit conversion from 'long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Werror,-Wimplicit-int-float-conversion] > > The warning will be enabled by default in clang 10. It is not > available for clang <= 9. > > The clamp is actually useless; @value is checked to be within > 0..MAX_MIGRATE_DOWNTIME_SECONDS immediately before. Delete it. > > While there, make the conversion from double to int64_t explicit. > > Signed-off-by: Fangrui Song <i@maskray.me> > Reviewed-by: Markus Armbruster <armbru@redhat.com> > [Patch split, commit message improved] > Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Should I get this through migration tree, or are you going to pull it? Later, Juan.
On 11/22/19 9:00 AM, Markus Armbruster wrote: > From: Fangrui Song <i@maskray.me> > > Clang does not like qmp_migrate_set_downtime()'s code to clamp double > @value to 0..INT64_MAX: > > qemu/migration/migration.c:2038:24: error: implicit conversion from 'long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Werror,-Wimplicit-int-float-conversion] > > The warning will be enabled by default in clang 10. It is not > available for clang <= 9. > > The clamp is actually useless; @value is checked to be within > 0..MAX_MIGRATE_DOWNTIME_SECONDS immediately before. Delete it. > > While there, make the conversion from double to int64_t explicit. > > Signed-off-by: Fangrui Song <i@maskray.me> > Reviewed-by: Markus Armbruster <armbru@redhat.com> > [Patch split, commit message improved] > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > migration/migration.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 354ad072fa..09b150663f 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -2035,11 +2035,10 @@ void qmp_migrate_set_downtime(double value, Error **errp) > } > > value *= 1000; /* Convert to milliseconds */ > - value = MAX(0, MIN(INT64_MAX, value)); > > MigrateSetParameters p = { > .has_downtime_limit = true, > - .downtime_limit = value, > + .downtime_limit = (int64_t)value, I agree with Eric a one line comment about the explicit cast is welcomed. We can still use 'git blame', find the last commit sha, then look at the commit description. But having it along the code is straightforward. Preferably with a comment (the maintainer queing this can amend it): Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > }; > > qmp_migrate_set_parameters(&p, errp); >
Juan Quintela <quintela@redhat.com> writes: > Markus Armbruster <armbru@redhat.com> wrote: >> From: Fangrui Song <i@maskray.me> >> >> Clang does not like qmp_migrate_set_downtime()'s code to clamp double >> @value to 0..INT64_MAX: >> >> qemu/migration/migration.c:2038:24: error: implicit conversion from 'long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Werror,-Wimplicit-int-float-conversion] >> >> The warning will be enabled by default in clang 10. It is not >> available for clang <= 9. >> >> The clamp is actually useless; @value is checked to be within >> 0..MAX_MIGRATE_DOWNTIME_SECONDS immediately before. Delete it. >> >> While there, make the conversion from double to int64_t explicit. >> >> Signed-off-by: Fangrui Song <i@maskray.me> >> Reviewed-by: Markus Armbruster <armbru@redhat.com> >> [Patch split, commit message improved] >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > Should I get this through migration tree, or are you going to pull it? Plase take this patch through the migration tree.
Markus Armbruster <armbru@redhat.com> writes: > Juan Quintela <quintela@redhat.com> writes: > >> Markus Armbruster <armbru@redhat.com> wrote: >>> From: Fangrui Song <i@maskray.me> >>> >>> Clang does not like qmp_migrate_set_downtime()'s code to clamp double >>> @value to 0..INT64_MAX: >>> >>> qemu/migration/migration.c:2038:24: error: implicit conversion from 'long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Werror,-Wimplicit-int-float-conversion] >>> >>> The warning will be enabled by default in clang 10. It is not >>> available for clang <= 9. >>> >>> The clamp is actually useless; @value is checked to be within >>> 0..MAX_MIGRATE_DOWNTIME_SECONDS immediately before. Delete it. >>> >>> While there, make the conversion from double to int64_t explicit. >>> >>> Signed-off-by: Fangrui Song <i@maskray.me> >>> Reviewed-by: Markus Armbruster <armbru@redhat.com> >>> [Patch split, commit message improved] >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> >> Reviewed-by: Juan Quintela <quintela@redhat.com> >> >> Should I get this through migration tree, or are you going to pull it? > > Plase take this patch through the migration tree. Ping...
Markus Armbruster <armbru@redhat.com> wrote: > Markus Armbruster <armbru@redhat.com> writes: > >> Juan Quintela <quintela@redhat.com> writes: >> >>> Markus Armbruster <armbru@redhat.com> wrote: >>>> From: Fangrui Song <i@maskray.me> >>>> >>>> Clang does not like qmp_migrate_set_downtime()'s code to clamp double >>>> @value to 0..INT64_MAX: >>>> >>>> qemu/migration/migration.c:2038:24: error: implicit conversion >>>> from 'long' to 'double' changes value from 9223372036854775807 to >>>> 9223372036854775808 [-Werror,-Wimplicit-int-float-conversion] >>>> >>>> The warning will be enabled by default in clang 10. It is not >>>> available for clang <= 9. >>>> >>>> The clamp is actually useless; @value is checked to be within >>>> 0..MAX_MIGRATE_DOWNTIME_SECONDS immediately before. Delete it. >>>> >>>> While there, make the conversion from double to int64_t explicit. >>>> >>>> Signed-off-by: Fangrui Song <i@maskray.me> >>>> Reviewed-by: Markus Armbruster <armbru@redhat.com> >>>> [Patch split, commit message improved] >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> >>> Reviewed-by: Juan Quintela <quintela@redhat.com> >>> >>> Should I get this through migration tree, or are you going to pull it? >> >> Plase take this patch through the migration tree. > > Ping... It was on my last pull request (with didn't work due to ...) And it is on the pull request sent 30 mins ago O:-) Later, Juan.
* Markus Armbruster (armbru@redhat.com) wrote: > Markus Armbruster <armbru@redhat.com> writes: > > > Juan Quintela <quintela@redhat.com> writes: > > > >> Markus Armbruster <armbru@redhat.com> wrote: > >>> From: Fangrui Song <i@maskray.me> > >>> > >>> Clang does not like qmp_migrate_set_downtime()'s code to clamp double > >>> @value to 0..INT64_MAX: > >>> > >>> qemu/migration/migration.c:2038:24: error: implicit conversion from 'long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Werror,-Wimplicit-int-float-conversion] > >>> > >>> The warning will be enabled by default in clang 10. It is not > >>> available for clang <= 9. > >>> > >>> The clamp is actually useless; @value is checked to be within > >>> 0..MAX_MIGRATE_DOWNTIME_SECONDS immediately before. Delete it. > >>> > >>> While there, make the conversion from double to int64_t explicit. > >>> > >>> Signed-off-by: Fangrui Song <i@maskray.me> > >>> Reviewed-by: Markus Armbruster <armbru@redhat.com> > >>> [Patch split, commit message improved] > >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> > >> > >> Reviewed-by: Juan Quintela <quintela@redhat.com> > >> > >> Should I get this through migration tree, or are you going to pull it? > > > > Plase take this patch through the migration tree. > > Ping... This looks like it went in in today's migration pull. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/migration/migration.c b/migration/migration.c index 354ad072fa..09b150663f 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -2035,11 +2035,10 @@ void qmp_migrate_set_downtime(double value, Error **errp) } value *= 1000; /* Convert to milliseconds */ - value = MAX(0, MIN(INT64_MAX, value)); MigrateSetParameters p = { .has_downtime_limit = true, - .downtime_limit = value, + .downtime_limit = (int64_t)value, }; qmp_migrate_set_parameters(&p, errp);