[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/8] cutils: unsigned int parsing functions
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH 1/8] cutils: unsigned int parsing functions |
Date: |
Wed, 16 Jan 2013 15:33:36 -0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
- [Qemu-devel] [PATCH 2/8] vl.c: Fix off-by-one bug when handling "-numa node" argument, (continued)
- [Qemu-devel] [PATCH 2/8] vl.c: Fix off-by-one bug when handling "-numa node" argument, Eduardo Habkost, 2013/01/16
- [Qemu-devel] [PATCH 7/8] vl.c: Extract -numa "cpus" parsing to separate function, Eduardo Habkost, 2013/01/16
- [Qemu-devel] [PATCH 3/8] vl.c: Abort on unknown -numa option type, Eduardo Habkost, 2013/01/16
- [Qemu-devel] [PATCH 8/8] vl.c: validate -numa "cpus" parameter properly, Eduardo Habkost, 2013/01/16
- [Qemu-devel] [PATCH 1/8] cutils: unsigned int parsing functions, Eduardo Habkost, 2013/01/16
[Qemu-devel] [PATCH 5/8] vl.c: numa_add(): Validate nodeid before using it, Eduardo Habkost, 2013/01/16
[Qemu-devel] [PATCH 4/8] vl.c: Check for NUMA node limit inside numa_add(), Eduardo Habkost, 2013/01/16