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

[Eric Blake]

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?

> +
> +    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

signature.asc
Description: OpenPGP digital signature