On 06/17/2014 11:41 AM, Michael S. Tsirkin wrote: > From: Hu Tao <hu...@cn.fujitsu.com> > > Signed-off-by: Hu Tao <hu...@cn.fujitsu.com> > Acked-by: Michael S. Tsirkin <m...@redhat.com> > Tested-by: Michael S. Tsirkin <m...@redhat.com> > Signed-off-by: Michael S. Tsirkin <m...@redhat.com> > > 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
