On Thu, 08/01 07:52, Eric Blake wrote:
> On 08/01/2013 12:31 AM, Fam Zheng wrote:
> > Fix it by calling strtoll instead, which will report ERANGE as expected.
> >
> > (HMP) block_set_io_throttle ide0-hd0 999999999999999999 0 0 0 0 0
> > (HMP) block_set_io_throttle ide0-hd0 9999999999999999999 0 0 0 0 0
> > number too large
> > (HMP) block_set_io_throttle ide0-hd0 99999999999999999999 0 0 0 0 0
> > number too large
>
> Your change causes this error message:
> (HMP) block_set_io_throttle ide0-hd0 -99999999999999999999 0 0 0 0 0
> number too large
>
> Does the "too large" mean in magnitude (correct message) or in value
> (misleading message, as any negative number is smaller in value than our
> minimum of 0)?
OK, it's another thing. If you try this w/o my patch:
(qemu) block_set_io_throttle ide0-hd0 -999999999999999999 0 0 0 0 0
bps and iops values must be 0 or greater
(qemu) block_set_io_throttle ide0-hd0 -9999999999999999999 0 0 0 0 0
/* Oops, no fail here? Of course it's because int64_t overflow (a
* negative negative) . */
(qemu) block_set_io_throttle ide0-hd0 -99999999999999999999 0 0 0 0 0
number too large
Because in expr_unary():
3233 case '-':
3234 next();
3235 n = -expr_unary(mon);
3236 break;
Then you know why, the nested expr_unary(mon) getting absolute part
reports too large...
>
> >
> > Signed-off-by: Fam Zheng <f...@redhat.com>
> > ---
> > monitor.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 5dc0aa9..7bfb469 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -3286,7 +3286,7 @@ static int64_t expr_unary(Monitor *mon)
> > break;
> > default:
> > errno = 0;
> > - n = strtoull(pch, &p, 0);
> > + n = strtoll(pch, &p, 0);
>
> I'm worried that this will break callers that treat their argument as
> unsigned, and where the full range of unsigned input was desirable. At
> this point, it's probably safer to do a case-by-case analysis of all
> callers that use expr_unary() to decide which callers must reject
> negative values, instead of making the parser reject numbers that it
> previously accepted, thus changing the behavior of callers that treated
> the result as unsigned.
>
You are right, there are callers cast it back to uint64_t, e.g.
hmp.c:735
uint32_t size = qdict_get_int(qdict, "size")
which means they could get number as large as 9999999999999999999. This
is tricky.
--
Fam
