Re: [Qemu-devel] [PULL 091/103] qapi: make string input visitor parse int list

[Michael S. Tsirkin]

On Tue, Jun 17, 2014 at 03:36:35PM -0600, Eric Blake wrote:
> On 06/17/2014 11:41 AM, Michael S. Tsirkin wrote:
> > From: Hu Tao <address@hidden>
> > 
> > Signed-off-by: Hu Tao <address@hidden>
> > Acked-by: Michael S. Tsirkin <address@hidden>
> > Tested-by: Michael S. Tsirkin <address@hidden>
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > 
> > MST: split up patch
> > ---
> >  qapi/string-input-visitor.c       | 195 
> > ++++++++++++++++++++++++++++++++++++--
> >  tests/test-string-input-visitor.c |  36 +++++++
> >  2 files changed, 223 insertions(+), 8 deletions(-)
> > 
> 
> > +static void parse_str(StringInputVisitor *siv, Error **errp)
> > +{
> > +    char *str = (char *) siv->string;
> > +    long long start, end;
> > +    Range *cur;
> > +    char *endptr;
> > +
> > +    if (siv->ranges) {
> > +        return;
> > +    }
> > +
> > +    errno = 0;
> > +    do {
> > +        start = strtoll(str, &endptr, 0);
> > +        if (errno == 0 && endptr > str && INT64_MIN <= start &&
> > +            start <= INT64_MAX) {
> 
> INT64_MIN <= start && start <= INT64_MAX is dead code (always true,
> unless you are on some weird platform where long long is larger than
> int64_t).
> 
> > +            if (*endptr == '\0') {
> > +                cur = g_malloc0(sizeof(*cur));
> > +                cur->begin = start;
> > +                cur->end = start + 1;
> > +                siv->ranges = g_list_insert_sorted_merged(siv->ranges, cur,
> > +                                                          range_compare);
> > +                cur = NULL;
> > +                str = NULL;
> > +            } else if (*endptr == '-') {
> > +                str = endptr + 1;
> > +                end = strtoll(str, &endptr, 0);
> > +                if (errno == 0 && endptr > str &&
> > +                    INT64_MIN <= end && end <= INT64_MAX && start <= end &&
> 
> Another dead line.
> 
> ...
> > +        } else {
> > +            goto error;
> > +        }
> > +    } while (str);
> 
> Your code has a bug.  You fail to reset errno = 0 prior to resuming the
> next iteration of the while loop, even though you have called
> intermediate functions that may have clobbered errno.  You'll need to
> submit a followup patch to fix it.
> 
> 
> > +static void test_visitor_in_intList(TestInputVisitorData *data,
> > +                                    const void *unused)
> > +{
> > +    int64_t value[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20};
> > +    int16List *res = NULL, *tmp;
> > +    Error *errp = NULL;
> > +    Visitor *v;
> > +    int i = 0;
> > +
> > +    v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
> 
> Should you also test the ability to create ranges of negative numbers,
> such as "-3--1", since integers are signed?

As the code uses unsigned Range internally, it does not currently work.
I agree we should add a patch to handle negative values and
fail gracefully, but this isn't the only place where we don't:
same applies to most input visitors.

Maybe we should change the default integer to do this check
automatically everywhere.

Are there any integer properties that need negative values?

> > +
> > +    visit_type_int16List(v, &res, NULL, &errp);
> > +    g_assert(errp == NULL);
> 
> It's shorter to write this as one line:
> 
> visit_type_int16List(v, &res, NULL, &error_abort);
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>