[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:56:11 -0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Wed, Jan 16, 2013 at 10:50:18AM -0700, Eric Blake wrote:
> On 01/16/2013 10:33 AM, Eduardo Habkost wrote:
> > 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().
> >>>
> >>
> >> 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.
>
> But see my comments in 8/8 about whether it makes sense to add a 'base'
> parameter to let the caller choose whether they want to allow octal or
> require strict decimal parsing.
In that case, I will add at least a test cases to check if "010" gives
the expected result depending on the 'base' parameter.
>
> >>> + r = -errno;
> >>
> >> Why two spaces?
> >
> > Typo. I will send a fixed version (keeping your Reviewed-by, if you
> > don't mind).
>
> No problem - I wouldn't have left a reviewed-by if I didn't think the
> changes couldn't be trivially fixed. On the other hand, you may have a
> non-trivial fix in the form of adding a base parameter...
I was planning to keep the reviewed-by line only if fixing the trivial
whitespace issue above. I won't keep it when adding the 'base'
parameter, don't worry. :-)
--
Eduardo
- [Qemu-devel] [PATCH 2/8] vl.c: Fix off-by-one bug when handling "-numa node" argument, (continued)
[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
Message not available