Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/2] blockdev: Error out on negative throttling option values

[Fam Zheng <famz@redhat.com>]

On Thu, 01/14 16:50, Max Reitz wrote:
> On 14.01.2016 16:46, Max Reitz wrote:
> > On 14.01.2016 05:08, Fam Zheng wrote:
> >> The implicit casting from unsigned int to double changes negative values
> >> into large positive numbers and accepts them.  We should instead print
> >> an error.
> >>
> >> Check the number range so this case is caught and reported.
> >>
> >> Signed-off-by: Fam Zheng <famz@redhat.com>
> >> ---
> >>  blockdev.c              |  3 ++-
> >>  include/qemu/throttle.h |  2 ++
> >>  util/throttle.c         | 16 ++++++----------
> >>  3 files changed, 10 insertions(+), 11 deletions(-)
> > 
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > 
> >> diff --git a/blockdev.c b/blockdev.c
> >> index 2df0c6d..1afef87 100644
> >> --- a/blockdev.c
> >> +++ b/blockdev.c
> >> @@ -348,7 +348,8 @@ static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
> >>      }
> >>  
> >>      if (!throttle_is_valid(cfg)) {
> >> -        error_setg(errp, "bps/iops/maxs values must be 0 or greater");
> >> +        error_setg(errp, "bps/iops/maxs values must be within [0, %" PRId64
> 
> Pre-existing, but you might want to fix this to "bps/iops/max" while
> touching this line.

Makes sense, I'll apply your rev-by and fix this in v4.

Fam

> 
> Max
> 
> >> +                         ")", (int64_t)THROTTLE_VALUE_MAX);
> > 
> > I personally would have liked a simpler %lli and no cast, but I can see
> > why you want an explicit int64_t here.
> > 
> >>          return false;
> >>      }
> >>  
> >> diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
> >> index 12faaad..d0c98ed 100644
> >> --- a/include/qemu/throttle.h
> >> +++ b/include/qemu/throttle.h
> >> @@ -29,6 +29,8 @@
> >>  #include "qemu-common.h"
> >>  #include "qemu/timer.h"
> >>  
> >> +#define THROTTLE_VALUE_MAX 1000000000000000LL
> > 
> > <pedantic>
> > But then you could use UINT64_C(1000000000000000) here.
> > </pedantic>
> > 
> 
>