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