Re: [Qemu-devel] [PATCH 1/8] cutils: unsigned int parsing functions

[Eduardo Habkost]

On Wed, Jan 16, 2013 at 10:10:42AM -0700, Eric Blake wrote:
> On 01/16/2013 08:24 AM, Eduardo Habkost wrote:
> > There are lots of duplicate parsing code using strto*() in QEMU, and
> > most of that code is broken in one way or another. Even the visitors
> > code have duplicate integer parsing code[1]. This introduces functions
> > to help parsing unsigned int values: parse_uint() and parse_uint_full().
> > 
> > Parsing functions for signed ints and floats will be submitted later.
> > 
> > parse_uint_full() has all the checks made by opts_type_uint64() at
> > opts-visitor.c:
> > 
> >  - Check for NULL (returns -EINVAL)
> >  - Check for negative numbers (returns -ERANGE)
> >  - Check for empty string (returns -EINVAL)
> >  - Check for overflow or other errno values set by strtoll() (returns
> >    -errno)
> >  - Check for end of string (reject invalid characters after number)
> >    (returns -EINVAL)
> > 
> > parse_uint() does everything above except checking for the end of the
> > string, so callers can continue parsing the remainder of string after
> > the number.
> > 
> > Unit tests included.
> > 
> > [1] string-input-visitor.c:parse_int() could use the same parsing code
> >     used by opts-visitor.c:opts_type_int(), instead of duplicating that
> >     logic.
> > 
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> > Cc: Laszlo Ersek <address@hidden>
> > Cc: Eric Blake <address@hidden>
> > ---
> 
> > +++ b/tests/test-cutils.c
> 
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a 
> > copy
> 
> Interesting that you chose a BSD license instead of GPL, but doesn't
> affect my review.

It's a personal preference, but by coincidence is also the license of
cutils.c.

> 
> Your test case lacks test of octal or hexadecimal input strings; is that
> worth adding?

I believe I trust strtoll() enough to not require tests for those cases.
Just like I didn't add tests involving trailing spaces or tabs, except
on the negative-number test case.


> > +++ b/util/cutils.c
> > @@ -270,6 +270,82 @@ int64_t strtosz(const char *nptr, char **end)
> >      return strtosz_suffix(nptr, end, STRTOSZ_DEFSUFFIX_MB);
> >  }
> >  
> > +/* Try to parse an unsigned integer
> > + *
> > + * Error checks done by the function:
> > + * - NULL pointer will return -EINVAL.
> > + * - Empty strings will return -EINVAL.
> > + * - Overflow errors or other errno values  set by strtoull() will
> > + *   return -errno (-ERANGE in case of overflow).
> > + * - Differently from strtoull(), values starting with a minus sign are
> > + *   rejected (returning -ERANGE).
> 
> Interesting that you chose to reject negative numbers, even though
> strtoull() is required to accept them.  But you documented it and tested
> for it, so I can live with it.

I find this behavior of strtoull() confusing for users. If an
option/field requires positive numbers, I expect QEMU to reject "-1"
instead of silently converting it to ULLONG_MAX.

> 
> > +    errno = 0;
> > +    val = strtoull(s, &endp, 0);
> > +    if (errno) {
> > +        r =  -errno;
> 
> Why two spaces?

Typo. I will send a fixed version (keeping your Reviewed-by, if you
don't mind).

> 
> Reviewed-by: Eric Blake <address@hidden>

Thanks!

-- 
Eduardo