Re: [Qemu-devel] [PATCH] monitor: fix parsing of big int

[Fam Zheng <famz@redhat.com>]

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 <famz@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