Patchwork [8/8] OptsVisitor: introduce unit tests, with test cases for range flattening

login
register
mail settings
Submitter Laszlo Ersek
Date July 22, 2013, 9:07 p.m.
Message ID <1374527256-27631-9-git-send-email-lersek@redhat.com>
Download mbox | patch
Permalink /patch/260821/
State New
Headers show

Comments

Laszlo Ersek - July 22, 2013, 9:07 p.m.
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 tests/Makefile            |    6 +-
 qapi-schema-test.json     |   15 +++
 tests/test-opts-visitor.c |  275 +++++++++++++++++++++++++++++++++++++++++++++
 .gitignore                |    1 +
 4 files changed, 296 insertions(+), 1 deletions(-)
 create mode 100644 tests/test-opts-visitor.c
Eric Blake - July 22, 2013, 10:04 p.m.
On 07/22/2013 03:07 PM, Laszlo Ersek wrote:
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  tests/Makefile            |    6 +-
>  qapi-schema-test.json     |   15 +++
>  tests/test-opts-visitor.c |  275 +++++++++++++++++++++++++++++++++++++++++++++
>  .gitignore                |    1 +
>  4 files changed, 296 insertions(+), 1 deletions(-)
>  create mode 100644 tests/test-opts-visitor.c

> +    add_test("/visitor/opts/i64/val1/errno",    &expect_fail,
> +             "i64=0x8000000000000000");
> +    add_test("/visitor/opts/i64/val1/empty",    &expect_fail, "i64=");
> +    add_test("/visitor/opts/i64/val1/trailing", &expect_fail, "i64=5z");
> +    add_test("/visitor/opts/i64/nonlist",       &expect_fail, "i64x=5-6");
> +    add_test("/visitor/opts/i64/val2/errno",    &expect_fail,
> +             "i64=0x7fffffffffffffff-0x8000000000000000");
> +    add_test("/visitor/opts/i64/val2/empty",    &expect_fail, "i64=5-");
> +    add_test("/visitor/opts/i64/val2/trailing", &expect_fail, "i64=5-6z");
> +    add_test("/visitor/opts/i64/range/empty",   &expect_fail, "i64=6-5");
> +    add_test("/visitor/opts/i64/range/minval",  &expect_i64_min,
> +             "i64=-0x8000000000000000--0x8000000000000000");
> +    add_test("/visitor/opts/i64/range/maxval",  &expect_i64_max,
> +             "i64=0x7fffffffffffffff-0x7fffffffffffffff");

Pretty thorough, although I thought of a couple other ideas to test:
i64=5z-6 should fail; i64=5-6-7 should fail
Laszlo Ersek - July 22, 2013, 10:24 p.m.
On 07/23/13 00:04, Eric Blake wrote:
> On 07/22/2013 03:07 PM, Laszlo Ersek wrote:
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  tests/Makefile            |    6 +-
>>  qapi-schema-test.json     |   15 +++
>>  tests/test-opts-visitor.c |  275 +++++++++++++++++++++++++++++++++++++++++++++
>>  .gitignore                |    1 +
>>  4 files changed, 296 insertions(+), 1 deletions(-)
>>  create mode 100644 tests/test-opts-visitor.c
> 
>> +    add_test("/visitor/opts/i64/val1/errno",    &expect_fail,
>> +             "i64=0x8000000000000000");
>> +    add_test("/visitor/opts/i64/val1/empty",    &expect_fail, "i64=");
>> +    add_test("/visitor/opts/i64/val1/trailing", &expect_fail, "i64=5z");
>> +    add_test("/visitor/opts/i64/nonlist",       &expect_fail, "i64x=5-6");
>> +    add_test("/visitor/opts/i64/val2/errno",    &expect_fail,
>> +             "i64=0x7fffffffffffffff-0x8000000000000000");
>> +    add_test("/visitor/opts/i64/val2/empty",    &expect_fail, "i64=5-");
>> +    add_test("/visitor/opts/i64/val2/trailing", &expect_fail, "i64=5-6z");
>> +    add_test("/visitor/opts/i64/range/empty",   &expect_fail, "i64=6-5");
>> +    add_test("/visitor/opts/i64/range/minval",  &expect_i64_min,
>> +             "i64=-0x8000000000000000--0x8000000000000000");
>> +    add_test("/visitor/opts/i64/range/maxval",  &expect_i64_max,
>> +             "i64=0x7fffffffffffffff-0x7fffffffffffffff");
> 
> Pretty thorough, although I thought of a couple other ideas to test:
> i64=5z-6 should fail; i64=5-6-7 should fail

I can add them if you insist, but I wrote (and single-stepped all of)
the test cases so that all branches added by patches 3, 5 and 6 would be
covered. (Some of the final tests in this function are actually
redundant, but I liked how they looked :))

For example, "i64=5z-6" is no different from "i64=5z", in patch 3 both
the first added (*endptr == '\0') condition and the (*endptr == '-')
fail the same way for both input strings: we never look past the "z".

Likewise, "i64=5-6-7" is the same case as "i64=5-6z": both characters
after the "6" (ie. "-" and "z") violate the second added (*endptr ==
'\0') condition in patch 3 the same way.

Do you accept this argument? :)

Thanks!
Laszlo
Eric Blake - July 22, 2013, 10:26 p.m.
On 07/22/2013 04:24 PM, Laszlo Ersek wrote:
>> Pretty thorough, although I thought of a couple other ideas to test:
>> i64=5z-6 should fail; i64=5-6-7 should fail
> 
> I can add them if you insist, but I wrote (and single-stepped all of)
> the test cases so that all branches added by patches 3, 5 and 6 would be
> covered. (Some of the final tests in this function are actually
> redundant, but I liked how they looked :))
> 
> For example, "i64=5z-6" is no different from "i64=5z", in patch 3 both
> the first added (*endptr == '\0') condition and the (*endptr == '-')
> fail the same way for both input strings: we never look past the "z".
> 
> Likewise, "i64=5-6-7" is the same case as "i64=5-6z": both characters
> after the "6" (ie. "-" and "z") violate the second added (*endptr ==
> '\0') condition in patch 3 the same way.
> 
> Do you accept this argument? :)

Yes, I can agree you have 100% code coverage as currently coded.  Adding
what currently forms redundant cases may avoid future patch-writers from
breaking 100% coverage while actually triggering different paths between
the cases; but at the same time, we can assume such a future
patch-writer would be adding some new feature to the parser, and could
expand the testsuite accordingly as part of their efforts.  So no, I
won't insist on a respin :)
Laszlo Ersek - July 22, 2013, 10:37 p.m.
On 07/23/13 00:26, Eric Blake wrote:
> On 07/22/2013 04:24 PM, Laszlo Ersek wrote:
>>> Pretty thorough, although I thought of a couple other ideas to test:
>>> i64=5z-6 should fail; i64=5-6-7 should fail
>>
>> I can add them if you insist, but I wrote (and single-stepped all of)
>> the test cases so that all branches added by patches 3, 5 and 6 would be
>> covered. (Some of the final tests in this function are actually
>> redundant, but I liked how they looked :))
>>
>> For example, "i64=5z-6" is no different from "i64=5z", in patch 3 both
>> the first added (*endptr == '\0') condition and the (*endptr == '-')
>> fail the same way for both input strings: we never look past the "z".
>>
>> Likewise, "i64=5-6-7" is the same case as "i64=5-6z": both characters
>> after the "6" (ie. "-" and "z") violate the second added (*endptr ==
>> '\0') condition in patch 3 the same way.
>>
>> Do you accept this argument? :)
> 
> Yes, I can agree you have 100% code coverage as currently coded.  Adding
> what currently forms redundant cases may avoid future patch-writers from
> breaking 100% coverage while actually triggering different paths between
> the cases; but at the same time, we can assume such a future
> patch-writer would be adding some new feature to the parser, and could
> expand the testsuite accordingly as part of their efforts.

Agreed!

> So no, I
> won't insist on a respin :)

Thank you very much :)

/me bows and scrapes
Laszlo
Luiz Capitulino - Aug. 19, 2013, 7:26 p.m.
On Mon, 22 Jul 2013 23:07:36 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

This patch now conflicts, can you respin please?

> ---
>  tests/Makefile            |    6 +-
>  qapi-schema-test.json     |   15 +++
>  tests/test-opts-visitor.c |  275 +++++++++++++++++++++++++++++++++++++++++++++
>  .gitignore                |    1 +
>  4 files changed, 296 insertions(+), 1 deletions(-)
>  create mode 100644 tests/test-opts-visitor.c
> 
> diff --git a/tests/Makefile b/tests/Makefile
> index 425a9a8..8bb290e 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -23,6 +23,8 @@ check-unit-y += tests/test-string-input-visitor$(EXESUF)
>  gcov-files-test-string-input-visitor-y = qapi/string-input-visitor.c
>  check-unit-y += tests/test-string-output-visitor$(EXESUF)
>  gcov-files-test-string-output-visitor-y = qapi/string-output-visitor.c
> +check-unit-y += tests/test-opts-visitor$(EXESUF)
> +gcov-files-test-opts-visitor-y = qapi/opts-visitor.c
>  check-unit-y += tests/test-coroutine$(EXESUF)
>  gcov-files-test-coroutine-y = coroutine-$(CONFIG_COROUTINE_BACKEND).c
>  check-unit-y += tests/test-visitor-serialization$(EXESUF)
> @@ -81,7 +83,8 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
>  	tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \
>  	tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \
>  	tests/test-qmp-commands.o tests/test-visitor-serialization.o \
> -	tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o
> +	tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o \
> +	tests/test-opts-visitor.o
>  
>  test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o
>  
> @@ -123,6 +126,7 @@ tests/test-qmp-input-visitor$(EXESUF): tests/test-qmp-input-visitor.o $(test-qap
>  tests/test-qmp-input-strict$(EXESUF): tests/test-qmp-input-strict.o $(test-qapi-obj-y) libqemuutil.a libqemustub.a
>  tests/test-qmp-commands$(EXESUF): tests/test-qmp-commands.o tests/test-qmp-marshal.o $(test-qapi-obj-y) qapi-types.o qapi-visit.o libqemuutil.a libqemustub.a
>  tests/test-visitor-serialization$(EXESUF): tests/test-visitor-serialization.o $(test-qapi-obj-y) libqemuutil.a libqemustub.a
> +tests/test-opts-visitor$(EXESUF): tests/test-opts-visitor.o $(test-qapi-obj-y) libqemuutil.a libqemustub.a
>  
>  tests/test-mul64$(EXESUF): tests/test-mul64.o libqemuutil.a
>  
> diff --git a/qapi-schema-test.json b/qapi-schema-test.json
> index 4434fa3..fe5af75 100644
> --- a/qapi-schema-test.json
> +++ b/qapi-schema-test.json
> @@ -51,3 +51,18 @@
>  { 'command': 'user_def_cmd', 'data': {} }
>  { 'command': 'user_def_cmd1', 'data': {'ud1a': 'UserDefOne'} }
>  { 'command': 'user_def_cmd2', 'data': {'ud1a': 'UserDefOne', 'ud1b': 'UserDefOne'}, 'returns': 'UserDefTwo' }
> +
> +# For testing integer range flattening in opts-visitor. The following schema
> +# corresponds to the option format:
> +#
> +# -userdef i64=3-6,i64=-5--1,u64=2,u16=1,u16=7-12
> +#
> +# For simplicity, this example doesn't use [type=]discriminator nor optargs
> +# specific to discriminator values.
> +{ 'type': 'UserDefOptions',
> +  'data': {
> +    '*i64' : [ 'int'    ],
> +    '*u64' : [ 'uint64' ],
> +    '*u16' : [ 'uint16' ],
> +    '*i64x':   'int'     ,
> +    '*u64x':   'uint64'  } }
> diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c
> new file mode 100644
> index 0000000..9f902b5
> --- /dev/null
> +++ b/tests/test-opts-visitor.c
> @@ -0,0 +1,275 @@
> +/*
> + * Options Visitor unit-tests.
> + *
> + * Copyright (C) 2013 Red Hat, Inc.
> + *
> + * Authors:
> + *   Laszlo Ersek <lersek@redhat.com> (based on test-string-output-visitor)
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include <glib.h>
> +
> +#include "qemu/config-file.h"     /* qemu_add_opts() */
> +#include "qemu/option.h"          /* qemu_opts_parse() */
> +#include "qapi/opts-visitor.h"    /* opts_visitor_new() */
> +#include "test-qapi-visit.h"      /* visit_type_UserDefOptions() */
> +#include "qapi/dealloc-visitor.h" /* qapi_dealloc_visitor_new() */
> +
> +static QemuOptsList userdef_opts = {
> +    .name = "userdef",
> +    .head = QTAILQ_HEAD_INITIALIZER(userdef_opts.head),
> +    .desc = { { 0 } } /* validated with OptsVisitor */
> +};
> +
> +/* fixture (= glib test case context) and test case manipulation */
> +
> +typedef struct OptsVisitorFixture {
> +    UserDefOptions *userdef;
> +    Error *err;
> +} OptsVisitorFixture;
> +
> +
> +static void
> +setup_fixture(OptsVisitorFixture *f, gconstpointer test_data)
> +{
> +    const char *opts_string = test_data;
> +    QemuOpts *opts;
> +    OptsVisitor *ov;
> +
> +    opts = qemu_opts_parse(qemu_find_opts("userdef"), opts_string, 0);
> +    g_assert(opts != NULL);
> +
> +    ov = opts_visitor_new(opts);
> +    visit_type_UserDefOptions(opts_get_visitor(ov), &f->userdef, NULL,
> +                              &f->err);
> +    opts_visitor_cleanup(ov);
> +    qemu_opts_del(opts);
> +}
> +
> +
> +static void
> +teardown_fixture(OptsVisitorFixture *f, gconstpointer test_data)
> +{
> +    if (f->userdef != NULL) {
> +        QapiDeallocVisitor *dv;
> +
> +        dv = qapi_dealloc_visitor_new();
> +        visit_type_UserDefOptions(qapi_dealloc_get_visitor(dv), &f->userdef,
> +                                  NULL, NULL);
> +        qapi_dealloc_visitor_cleanup(dv);
> +    }
> +    error_free(f->err);
> +}
> +
> +
> +static void
> +add_test(const char *testpath,
> +         void (*test_func)(OptsVisitorFixture *f, gconstpointer test_data),
> +         gconstpointer test_data)
> +{
> +    g_test_add(testpath, OptsVisitorFixture, test_data, setup_fixture,
> +               test_func, teardown_fixture);
> +}
> +
> +/* test output evaluation */
> +
> +static void
> +expect_ok(OptsVisitorFixture *f, gconstpointer test_data)
> +{
> +    g_assert(f->err == NULL);
> +    g_assert(f->userdef != NULL);
> +}
> +
> +
> +static void
> +expect_fail(OptsVisitorFixture *f, gconstpointer test_data)
> +{
> +    g_assert(f->err != NULL);
> +
> +    /* The error message is printed when this test utility is invoked directly
> +     * (ie. without gtester) and the --verbose flag is passed:
> +     *
> +     * tests/test-opts-visitor --verbose
> +     */
> +    g_test_message("'%s': %s", (const char *)test_data,
> +                   error_get_pretty(f->err));
> +}
> +
> +
> +static void
> +test_value(OptsVisitorFixture *f, gconstpointer test_data)
> +{
> +    uint64_t magic, bitval;
> +    intList *i64;
> +    uint64List *u64;
> +    uint16List *u16;
> +
> +    expect_ok(f, test_data);
> +
> +    magic = 0;
> +    for (i64 = f->userdef->i64; i64 != NULL; i64 = i64->next) {
> +        g_assert(-16 <= i64->value && i64->value < 64-16);
> +        bitval = 1ull << (i64->value + 16);
> +        g_assert((magic & bitval) == 0);
> +        magic |= bitval;
> +    }
> +    g_assert(magic == 0xDEADBEEF);
> +
> +    magic = 0;
> +    for (u64 = f->userdef->u64; u64 != NULL; u64 = u64->next) {
> +        g_assert(u64->value < 64);
> +        bitval = 1ull << u64->value;
> +        g_assert((magic & bitval) == 0);
> +        magic |= bitval;
> +    }
> +    g_assert(magic == 0xBADC0FFEE0DDF00D);
> +
> +    magic = 0;
> +    for (u16 = f->userdef->u16; u16 != NULL; u16 = u16->next) {
> +        g_assert(u16->value < 64);
> +        bitval = 1ull << u16->value;
> +        g_assert((magic & bitval) == 0);
> +        magic |= bitval;
> +    }
> +    g_assert(magic == 0xD15EA5E);
> +}
> +
> +
> +static void
> +expect_i64_min(OptsVisitorFixture *f, gconstpointer test_data)
> +{
> +    expect_ok(f, test_data);
> +    g_assert(f->userdef->has_i64);
> +    g_assert(f->userdef->i64->next == NULL);
> +    g_assert(f->userdef->i64->value == INT64_MIN);
> +}
> +
> +
> +static void
> +expect_i64_max(OptsVisitorFixture *f, gconstpointer test_data)
> +{
> +    expect_ok(f, test_data);
> +    g_assert(f->userdef->has_i64);
> +    g_assert(f->userdef->i64->next == NULL);
> +    g_assert(f->userdef->i64->value == INT64_MAX);
> +}
> +
> +
> +static void
> +expect_zero(OptsVisitorFixture *f, gconstpointer test_data)
> +{
> +    expect_ok(f, test_data);
> +    g_assert(f->userdef->has_u64);
> +    g_assert(f->userdef->u64->next == NULL);
> +    g_assert(f->userdef->u64->value == 0);
> +}
> +
> +
> +static void
> +expect_u64_max(OptsVisitorFixture *f, gconstpointer test_data)
> +{
> +    expect_ok(f, test_data);
> +    g_assert(f->userdef->has_u64);
> +    g_assert(f->userdef->u64->next == NULL);
> +    g_assert(f->userdef->u64->value == UINT64_MAX);
> +}
> +
> +/* test cases */
> +
> +int
> +main(int argc, char **argv)
> +{
> +    g_test_init(&argc, &argv, NULL);
> +
> +    qemu_add_opts(&userdef_opts);
> +
> +    /* Three hexadecimal magic numbers, "dead beef", "bad coffee, odd food" and
> +     * "disease", from
> +     * <http://en.wikipedia.org/wiki/Magic_number_%28programming%29>, were
> +     * converted to binary and dissected into bit ranges. Each magic number is
> +     * going to be recomposed using the lists called "i64", "u64" and "u16",
> +     * respectively.
> +     *
> +     * (Note that these types pertain to the individual bit shift counts, not
> +     * the magic numbers themselves; the intent is to exercise opts_type_int()
> +     * and opts_type_uint64().)
> +     *
> +     * The "i64" shift counts have been decreased by 16 (decimal) in order to
> +     * test negative values as well. Finally, the full list of QemuOpt elements
> +     * has been permuted with "shuf".
> +     *
> +     * Both "i64" and "u64" have some (distinct) single-element ranges
> +     * represented as both "a" and "a-a". "u16" is a special case of "i64" (see
> +     * visit_type_uint16()), so it wouldn't add a separate test in this regard.
> +     */
> +
> +    add_test("/visitor/opts/flatten/value", &test_value,
> +             "i64=-1-0,u64=12-16,u64=2-3,i64=-11--9,u64=57,u16=9,i64=5-5,"
> +             "u16=1-4,u16=20,u64=63-63,i64=-16--13,u64=50-52,i64=14-15,u16=11,"
> +             "i64=7,u16=18,i64=2-3,u16=6,u64=54-55,u64=0,u64=18-20,u64=33-43,"
> +             "i64=9-12,u16=26-27,u64=59-61,u16=13-16,u64=29-31,u64=22-23,"
> +             "u16=24,i64=-7--3");
> +
> +    add_test("/visitor/opts/i64/val1/errno",    &expect_fail,
> +             "i64=0x8000000000000000");
> +    add_test("/visitor/opts/i64/val1/empty",    &expect_fail, "i64=");
> +    add_test("/visitor/opts/i64/val1/trailing", &expect_fail, "i64=5z");
> +    add_test("/visitor/opts/i64/nonlist",       &expect_fail, "i64x=5-6");
> +    add_test("/visitor/opts/i64/val2/errno",    &expect_fail,
> +             "i64=0x7fffffffffffffff-0x8000000000000000");
> +    add_test("/visitor/opts/i64/val2/empty",    &expect_fail, "i64=5-");
> +    add_test("/visitor/opts/i64/val2/trailing", &expect_fail, "i64=5-6z");
> +    add_test("/visitor/opts/i64/range/empty",   &expect_fail, "i64=6-5");
> +    add_test("/visitor/opts/i64/range/minval",  &expect_i64_min,
> +             "i64=-0x8000000000000000--0x8000000000000000");
> +    add_test("/visitor/opts/i64/range/maxval",  &expect_i64_max,
> +             "i64=0x7fffffffffffffff-0x7fffffffffffffff");
> +
> +    add_test("/visitor/opts/u64/val1/errno",    &expect_fail, "u64=-1");
> +    add_test("/visitor/opts/u64/val1/empty",    &expect_fail, "u64=");
> +    add_test("/visitor/opts/u64/val1/trailing", &expect_fail, "u64=5z");
> +    add_test("/visitor/opts/u64/nonlist",       &expect_fail, "u64x=5-6");
> +    add_test("/visitor/opts/u64/val2/errno",    &expect_fail,
> +             "u64=0xffffffffffffffff-0x10000000000000000");
> +    add_test("/visitor/opts/u64/val2/empty",    &expect_fail, "u64=5-");
> +    add_test("/visitor/opts/u64/val2/trailing", &expect_fail, "u64=5-6z");
> +    add_test("/visitor/opts/u64/range/empty",   &expect_fail, "u64=6-5");
> +    add_test("/visitor/opts/u64/range/minval",  &expect_zero, "u64=0-0");
> +    add_test("/visitor/opts/u64/range/maxval",  &expect_u64_max,
> +             "u64=0xffffffffffffffff-0xffffffffffffffff");
> +
> +    /* Test maximum range sizes. The macro value is open-coded here
> +     * *intentionally*; the test case must use concrete values by design. If
> +     * OPTS_VISITOR_RANGE_MAX is changed, the following values need to be
> +     * recalculated as well. The assert and this comment should help with it.
> +     */
> +    g_assert(OPTS_VISITOR_RANGE_MAX == 65536);
> +
> +    /* The unsigned case is simple, a u64-u64 difference can always be
> +     * represented as a u64.
> +     */
> +    add_test("/visitor/opts/u64/range/max",  &expect_ok,   "u64=0-65535");
> +    add_test("/visitor/opts/u64/range/2big", &expect_fail, "u64=0-65536");
> +
> +    /* The same cannot be said about an i64-i64 difference. */
> +    add_test("/visitor/opts/i64/range/max/pos/a", &expect_ok,
> +             "i64=0x7fffffffffff0000-0x7fffffffffffffff");
> +    add_test("/visitor/opts/i64/range/max/pos/b", &expect_ok,
> +             "i64=0x7ffffffffffeffff-0x7ffffffffffffffe");
> +    add_test("/visitor/opts/i64/range/2big/pos",  &expect_fail,
> +             "i64=0x7ffffffffffeffff-0x7fffffffffffffff");
> +    add_test("/visitor/opts/i64/range/max/neg/a", &expect_ok,
> +             "i64=-0x8000000000000000--0x7fffffffffff0001");
> +    add_test("/visitor/opts/i64/range/max/neg/b", &expect_ok,
> +             "i64=-0x7fffffffffffffff--0x7fffffffffff0000");
> +    add_test("/visitor/opts/i64/range/2big/neg",  &expect_fail,
> +             "i64=-0x8000000000000000--0x7fffffffffff0000");
> +    add_test("/visitor/opts/i64/range/2big/full", &expect_fail,
> +             "i64=-0x8000000000000000-0x7fffffffffffffff");
> +
> +    g_test_run();
> +    return 0;
> +}
> diff --git a/.gitignore b/.gitignore
> index 388cb45..60a0363 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -47,6 +47,7 @@ vscclient
>  QMP/qmp-commands.txt
>  test-coroutine
>  test-int128
> +test-opts-visitor
>  test-qmp-input-visitor
>  test-qmp-output-visitor
>  test-string-input-visitor
Laszlo Ersek - Aug. 19, 2013, 7:55 p.m.
On 08/19/13 21:26, Luiz Capitulino wrote:
> On Mon, 22 Jul 2013 23:07:36 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> 
> This patch now conflicts, can you respin please?

Can you retry with "git am -3"?

"git rebase -i" didn't ask me to do anything manually, so I'm guessing
if you let "git am" to do a 3-way merge, it should work.

(Of course I don't have anything against posting a v2, this is just an
attempt to keep the traffic low.)

Thanks!
Laszlo
Laszlo Ersek - Aug. 19, 2013, 8:04 p.m.
On 08/19/13 21:55, Laszlo Ersek wrote:
> On 08/19/13 21:26, Luiz Capitulino wrote:
>> On Mon, 22 Jul 2013 23:07:36 +0200
>> Laszlo Ersek <lersek@redhat.com> wrote:
>>
>>>
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>
>> This patch now conflicts, can you respin please?
> 
> Can you retry with "git am -3"?
> 
> "git rebase -i" didn't ask me to do anything manually, so I'm guessing
> if you let "git am" to do a 3-way merge, it should work.
> 
> (Of course I don't have anything against posting a v2, this is just an
> attempt to keep the traffic low.)

... yeah it looks like:

- a trivial context change in "tests/Makefile" (due to commit 3464700f),

- "qapi-schema-test.json" has been renamed to
"tests/qapi-schema/qapi-schema-test.json" (commit 4f193e34):

diff --git a/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
similarity index 100%
rename from qapi-schema-test.json
rename to tests/qapi-schema/qapi-schema-test.json

These should be no problem for "git am -3".

Thanks,
Laszlo

Patch

diff --git a/tests/Makefile b/tests/Makefile
index 425a9a8..8bb290e 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -23,6 +23,8 @@  check-unit-y += tests/test-string-input-visitor$(EXESUF)
 gcov-files-test-string-input-visitor-y = qapi/string-input-visitor.c
 check-unit-y += tests/test-string-output-visitor$(EXESUF)
 gcov-files-test-string-output-visitor-y = qapi/string-output-visitor.c
+check-unit-y += tests/test-opts-visitor$(EXESUF)
+gcov-files-test-opts-visitor-y = qapi/opts-visitor.c
 check-unit-y += tests/test-coroutine$(EXESUF)
 gcov-files-test-coroutine-y = coroutine-$(CONFIG_COROUTINE_BACKEND).c
 check-unit-y += tests/test-visitor-serialization$(EXESUF)
@@ -81,7 +83,8 @@  test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
 	tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \
 	tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \
 	tests/test-qmp-commands.o tests/test-visitor-serialization.o \
-	tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o
+	tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o \
+	tests/test-opts-visitor.o
 
 test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o
 
@@ -123,6 +126,7 @@  tests/test-qmp-input-visitor$(EXESUF): tests/test-qmp-input-visitor.o $(test-qap
 tests/test-qmp-input-strict$(EXESUF): tests/test-qmp-input-strict.o $(test-qapi-obj-y) libqemuutil.a libqemustub.a
 tests/test-qmp-commands$(EXESUF): tests/test-qmp-commands.o tests/test-qmp-marshal.o $(test-qapi-obj-y) qapi-types.o qapi-visit.o libqemuutil.a libqemustub.a
 tests/test-visitor-serialization$(EXESUF): tests/test-visitor-serialization.o $(test-qapi-obj-y) libqemuutil.a libqemustub.a
+tests/test-opts-visitor$(EXESUF): tests/test-opts-visitor.o $(test-qapi-obj-y) libqemuutil.a libqemustub.a
 
 tests/test-mul64$(EXESUF): tests/test-mul64.o libqemuutil.a
 
diff --git a/qapi-schema-test.json b/qapi-schema-test.json
index 4434fa3..fe5af75 100644
--- a/qapi-schema-test.json
+++ b/qapi-schema-test.json
@@ -51,3 +51,18 @@ 
 { 'command': 'user_def_cmd', 'data': {} }
 { 'command': 'user_def_cmd1', 'data': {'ud1a': 'UserDefOne'} }
 { 'command': 'user_def_cmd2', 'data': {'ud1a': 'UserDefOne', 'ud1b': 'UserDefOne'}, 'returns': 'UserDefTwo' }
+
+# For testing integer range flattening in opts-visitor. The following schema
+# corresponds to the option format:
+#
+# -userdef i64=3-6,i64=-5--1,u64=2,u16=1,u16=7-12
+#
+# For simplicity, this example doesn't use [type=]discriminator nor optargs
+# specific to discriminator values.
+{ 'type': 'UserDefOptions',
+  'data': {
+    '*i64' : [ 'int'    ],
+    '*u64' : [ 'uint64' ],
+    '*u16' : [ 'uint16' ],
+    '*i64x':   'int'     ,
+    '*u64x':   'uint64'  } }
diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c
new file mode 100644
index 0000000..9f902b5
--- /dev/null
+++ b/tests/test-opts-visitor.c
@@ -0,0 +1,275 @@ 
+/*
+ * Options Visitor unit-tests.
+ *
+ * Copyright (C) 2013 Red Hat, Inc.
+ *
+ * Authors:
+ *   Laszlo Ersek <lersek@redhat.com> (based on test-string-output-visitor)
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include <glib.h>
+
+#include "qemu/config-file.h"     /* qemu_add_opts() */
+#include "qemu/option.h"          /* qemu_opts_parse() */
+#include "qapi/opts-visitor.h"    /* opts_visitor_new() */
+#include "test-qapi-visit.h"      /* visit_type_UserDefOptions() */
+#include "qapi/dealloc-visitor.h" /* qapi_dealloc_visitor_new() */
+
+static QemuOptsList userdef_opts = {
+    .name = "userdef",
+    .head = QTAILQ_HEAD_INITIALIZER(userdef_opts.head),
+    .desc = { { 0 } } /* validated with OptsVisitor */
+};
+
+/* fixture (= glib test case context) and test case manipulation */
+
+typedef struct OptsVisitorFixture {
+    UserDefOptions *userdef;
+    Error *err;
+} OptsVisitorFixture;
+
+
+static void
+setup_fixture(OptsVisitorFixture *f, gconstpointer test_data)
+{
+    const char *opts_string = test_data;
+    QemuOpts *opts;
+    OptsVisitor *ov;
+
+    opts = qemu_opts_parse(qemu_find_opts("userdef"), opts_string, 0);
+    g_assert(opts != NULL);
+
+    ov = opts_visitor_new(opts);
+    visit_type_UserDefOptions(opts_get_visitor(ov), &f->userdef, NULL,
+                              &f->err);
+    opts_visitor_cleanup(ov);
+    qemu_opts_del(opts);
+}
+
+
+static void
+teardown_fixture(OptsVisitorFixture *f, gconstpointer test_data)
+{
+    if (f->userdef != NULL) {
+        QapiDeallocVisitor *dv;
+
+        dv = qapi_dealloc_visitor_new();
+        visit_type_UserDefOptions(qapi_dealloc_get_visitor(dv), &f->userdef,
+                                  NULL, NULL);
+        qapi_dealloc_visitor_cleanup(dv);
+    }
+    error_free(f->err);
+}
+
+
+static void
+add_test(const char *testpath,
+         void (*test_func)(OptsVisitorFixture *f, gconstpointer test_data),
+         gconstpointer test_data)
+{
+    g_test_add(testpath, OptsVisitorFixture, test_data, setup_fixture,
+               test_func, teardown_fixture);
+}
+
+/* test output evaluation */
+
+static void
+expect_ok(OptsVisitorFixture *f, gconstpointer test_data)
+{
+    g_assert(f->err == NULL);
+    g_assert(f->userdef != NULL);
+}
+
+
+static void
+expect_fail(OptsVisitorFixture *f, gconstpointer test_data)
+{
+    g_assert(f->err != NULL);
+
+    /* The error message is printed when this test utility is invoked directly
+     * (ie. without gtester) and the --verbose flag is passed:
+     *
+     * tests/test-opts-visitor --verbose
+     */
+    g_test_message("'%s': %s", (const char *)test_data,
+                   error_get_pretty(f->err));
+}
+
+
+static void
+test_value(OptsVisitorFixture *f, gconstpointer test_data)
+{
+    uint64_t magic, bitval;
+    intList *i64;
+    uint64List *u64;
+    uint16List *u16;
+
+    expect_ok(f, test_data);
+
+    magic = 0;
+    for (i64 = f->userdef->i64; i64 != NULL; i64 = i64->next) {
+        g_assert(-16 <= i64->value && i64->value < 64-16);
+        bitval = 1ull << (i64->value + 16);
+        g_assert((magic & bitval) == 0);
+        magic |= bitval;
+    }
+    g_assert(magic == 0xDEADBEEF);
+
+    magic = 0;
+    for (u64 = f->userdef->u64; u64 != NULL; u64 = u64->next) {
+        g_assert(u64->value < 64);
+        bitval = 1ull << u64->value;
+        g_assert((magic & bitval) == 0);
+        magic |= bitval;
+    }
+    g_assert(magic == 0xBADC0FFEE0DDF00D);
+
+    magic = 0;
+    for (u16 = f->userdef->u16; u16 != NULL; u16 = u16->next) {
+        g_assert(u16->value < 64);
+        bitval = 1ull << u16->value;
+        g_assert((magic & bitval) == 0);
+        magic |= bitval;
+    }
+    g_assert(magic == 0xD15EA5E);
+}
+
+
+static void
+expect_i64_min(OptsVisitorFixture *f, gconstpointer test_data)
+{
+    expect_ok(f, test_data);
+    g_assert(f->userdef->has_i64);
+    g_assert(f->userdef->i64->next == NULL);
+    g_assert(f->userdef->i64->value == INT64_MIN);
+}
+
+
+static void
+expect_i64_max(OptsVisitorFixture *f, gconstpointer test_data)
+{
+    expect_ok(f, test_data);
+    g_assert(f->userdef->has_i64);
+    g_assert(f->userdef->i64->next == NULL);
+    g_assert(f->userdef->i64->value == INT64_MAX);
+}
+
+
+static void
+expect_zero(OptsVisitorFixture *f, gconstpointer test_data)
+{
+    expect_ok(f, test_data);
+    g_assert(f->userdef->has_u64);
+    g_assert(f->userdef->u64->next == NULL);
+    g_assert(f->userdef->u64->value == 0);
+}
+
+
+static void
+expect_u64_max(OptsVisitorFixture *f, gconstpointer test_data)
+{
+    expect_ok(f, test_data);
+    g_assert(f->userdef->has_u64);
+    g_assert(f->userdef->u64->next == NULL);
+    g_assert(f->userdef->u64->value == UINT64_MAX);
+}
+
+/* test cases */
+
+int
+main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    qemu_add_opts(&userdef_opts);
+
+    /* Three hexadecimal magic numbers, "dead beef", "bad coffee, odd food" and
+     * "disease", from
+     * <http://en.wikipedia.org/wiki/Magic_number_%28programming%29>, were
+     * converted to binary and dissected into bit ranges. Each magic number is
+     * going to be recomposed using the lists called "i64", "u64" and "u16",
+     * respectively.
+     *
+     * (Note that these types pertain to the individual bit shift counts, not
+     * the magic numbers themselves; the intent is to exercise opts_type_int()
+     * and opts_type_uint64().)
+     *
+     * The "i64" shift counts have been decreased by 16 (decimal) in order to
+     * test negative values as well. Finally, the full list of QemuOpt elements
+     * has been permuted with "shuf".
+     *
+     * Both "i64" and "u64" have some (distinct) single-element ranges
+     * represented as both "a" and "a-a". "u16" is a special case of "i64" (see
+     * visit_type_uint16()), so it wouldn't add a separate test in this regard.
+     */
+
+    add_test("/visitor/opts/flatten/value", &test_value,
+             "i64=-1-0,u64=12-16,u64=2-3,i64=-11--9,u64=57,u16=9,i64=5-5,"
+             "u16=1-4,u16=20,u64=63-63,i64=-16--13,u64=50-52,i64=14-15,u16=11,"
+             "i64=7,u16=18,i64=2-3,u16=6,u64=54-55,u64=0,u64=18-20,u64=33-43,"
+             "i64=9-12,u16=26-27,u64=59-61,u16=13-16,u64=29-31,u64=22-23,"
+             "u16=24,i64=-7--3");
+
+    add_test("/visitor/opts/i64/val1/errno",    &expect_fail,
+             "i64=0x8000000000000000");
+    add_test("/visitor/opts/i64/val1/empty",    &expect_fail, "i64=");
+    add_test("/visitor/opts/i64/val1/trailing", &expect_fail, "i64=5z");
+    add_test("/visitor/opts/i64/nonlist",       &expect_fail, "i64x=5-6");
+    add_test("/visitor/opts/i64/val2/errno",    &expect_fail,
+             "i64=0x7fffffffffffffff-0x8000000000000000");
+    add_test("/visitor/opts/i64/val2/empty",    &expect_fail, "i64=5-");
+    add_test("/visitor/opts/i64/val2/trailing", &expect_fail, "i64=5-6z");
+    add_test("/visitor/opts/i64/range/empty",   &expect_fail, "i64=6-5");
+    add_test("/visitor/opts/i64/range/minval",  &expect_i64_min,
+             "i64=-0x8000000000000000--0x8000000000000000");
+    add_test("/visitor/opts/i64/range/maxval",  &expect_i64_max,
+             "i64=0x7fffffffffffffff-0x7fffffffffffffff");
+
+    add_test("/visitor/opts/u64/val1/errno",    &expect_fail, "u64=-1");
+    add_test("/visitor/opts/u64/val1/empty",    &expect_fail, "u64=");
+    add_test("/visitor/opts/u64/val1/trailing", &expect_fail, "u64=5z");
+    add_test("/visitor/opts/u64/nonlist",       &expect_fail, "u64x=5-6");
+    add_test("/visitor/opts/u64/val2/errno",    &expect_fail,
+             "u64=0xffffffffffffffff-0x10000000000000000");
+    add_test("/visitor/opts/u64/val2/empty",    &expect_fail, "u64=5-");
+    add_test("/visitor/opts/u64/val2/trailing", &expect_fail, "u64=5-6z");
+    add_test("/visitor/opts/u64/range/empty",   &expect_fail, "u64=6-5");
+    add_test("/visitor/opts/u64/range/minval",  &expect_zero, "u64=0-0");
+    add_test("/visitor/opts/u64/range/maxval",  &expect_u64_max,
+             "u64=0xffffffffffffffff-0xffffffffffffffff");
+
+    /* Test maximum range sizes. The macro value is open-coded here
+     * *intentionally*; the test case must use concrete values by design. If
+     * OPTS_VISITOR_RANGE_MAX is changed, the following values need to be
+     * recalculated as well. The assert and this comment should help with it.
+     */
+    g_assert(OPTS_VISITOR_RANGE_MAX == 65536);
+
+    /* The unsigned case is simple, a u64-u64 difference can always be
+     * represented as a u64.
+     */
+    add_test("/visitor/opts/u64/range/max",  &expect_ok,   "u64=0-65535");
+    add_test("/visitor/opts/u64/range/2big", &expect_fail, "u64=0-65536");
+
+    /* The same cannot be said about an i64-i64 difference. */
+    add_test("/visitor/opts/i64/range/max/pos/a", &expect_ok,
+             "i64=0x7fffffffffff0000-0x7fffffffffffffff");
+    add_test("/visitor/opts/i64/range/max/pos/b", &expect_ok,
+             "i64=0x7ffffffffffeffff-0x7ffffffffffffffe");
+    add_test("/visitor/opts/i64/range/2big/pos",  &expect_fail,
+             "i64=0x7ffffffffffeffff-0x7fffffffffffffff");
+    add_test("/visitor/opts/i64/range/max/neg/a", &expect_ok,
+             "i64=-0x8000000000000000--0x7fffffffffff0001");
+    add_test("/visitor/opts/i64/range/max/neg/b", &expect_ok,
+             "i64=-0x7fffffffffffffff--0x7fffffffffff0000");
+    add_test("/visitor/opts/i64/range/2big/neg",  &expect_fail,
+             "i64=-0x8000000000000000--0x7fffffffffff0000");
+    add_test("/visitor/opts/i64/range/2big/full", &expect_fail,
+             "i64=-0x8000000000000000-0x7fffffffffffffff");
+
+    g_test_run();
+    return 0;
+}
diff --git a/.gitignore b/.gitignore
index 388cb45..60a0363 100644
--- a/.gitignore
+++ b/.gitignore
@@ -47,6 +47,7 @@  vscclient
 QMP/qmp-commands.txt
 test-coroutine
 test-int128
+test-opts-visitor
 test-qmp-input-visitor
 test-qmp-output-visitor
 test-string-input-visitor