diff mbox

[7/7] qapi: support nested structs in OptsVisitor

Message ID 828b3ad8c9f32d5938f39068328d0794cfd9abf4.1441627176.git.DirtY.iCE.hu@gmail.com
State New
Headers show

Commit Message

=?UTF-8?B?Wm9sdMOhbiBLxZF2w6Fnw7M=?= Sept. 7, 2015, 12:14 p.m. UTC 
The current OptsVisitor flattens the whole structure, if there are same
named fields under different paths (like `in' and `out' in `Audiodev'),
the current visitor can't cope with them (for example setting
`frequency=44100' will set the in's frequency to 44100 and leave out's
frequency unspecified).

This patch fixes it, by always requiring a complete path in case of
nested structs.  Fields in the path are separated by dots, similar to C
structs (without pointers), like `in.frequency' or `out.frequency'.

You must provide a full path even in non-ambiguous cases.  The qapi
flattening commits hopefully ensures that this change doesn't create
backward compatibility problems.

Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
---
 qapi/opts-visitor.c                     | 116 ++++++++++++++++++++++++++------
 tests/qapi-schema/qapi-schema-test.json |   9 ++-
 tests/qapi-schema/qapi-schema-test.out  |   6 +-
 tests/test-opts-visitor.c               |  34 ++++++++++
 4 files changed, 141 insertions(+), 24 deletions(-)

Comments

Eric Blake Sept. 17, 2015, 9:24 p.m. UTC | #1 
On 09/07/2015 06:14 AM, Kővágó, Zoltán wrote:
> The current OptsVisitor flattens the whole structure, if there are same
> named fields under different paths (like `in' and `out' in `Audiodev'),
> the current visitor can't cope with them (for example setting
> `frequency=44100' will set the in's frequency to 44100 and leave out's
> frequency unspecified).
> 
> This patch fixes it, by always requiring a complete path in case of
> nested structs.  Fields in the path are separated by dots, similar to C
> structs (without pointers), like `in.frequency' or `out.frequency'.
> 
> You must provide a full path even in non-ambiguous cases.  The qapi
> flattening commits hopefully ensures that this change doesn't create
> backward compatibility problems.

It's the "hopefully" that worries me.

If I understand correctly, prior to this series, we have several
instances of qapi simple unions, where a member is a substruct when
converting to QObject.  Conceptually:

{ 'struct':'Sub', 'data':{ 'c':'int' } }
{ 'union':'Main', 'data':{ 'X':'Sub' } }

would accept the following QDict:

{ "type":"X", "data":{ "c":1 } }

and directly translating that to QemuOpts would be spelled:

-opt type=X,data.c=1

but we wanted shorthand:

-opt type=X,c=1

So, the "flattening" that opts-visitor does is what allows "c" instead
of "data.c" to refer to a nested member of a union.  It worked as long
as 'c' appeared only once per visit of the overall qapi type (multiple
branches of the union may have 'c', but for a given union branch, 'c'
appears only once no matter what depth of substructs it is reached through).

Question: do we actually ALLOW the user to specify data.c, or do we ONLY
accept the shorthand?


This series then proceeds to teach opts-visitor how to handle flat
unions.  Converting the above example to a flat union is done by:

{ 'enum':'Foo', 'data':['X'] }
{ 'struct':'Base', 'data':{ 'type':'Foo' } }
{ 'struct':'Sub', 'data':{ 'c':'int' } }
{ 'union':'Main', 'base':'Base', 'discriminator':'type',
  'data':{ 'X':'Sub' } }

which now accepts the following QDict:

{ "type":"X", "c":1" }

(note the loss of the nested 'data' struct), and directly translating
that to QemuOpts would be spelled:

-opt type=X,c=1

which exactly matches the shorthand that we previously accepted.  It
completely gets rid of the data.c longhand, but I don't know if we
accepted that in the first place.

So, the next question is whether we still need flattening.  If we no
longer have any simple unions parsed by QemuOpts, then the flattening no
longer helps us.  Furthermore, you have a case in audio where flattening
hurts; the QDict:

{ "in":{ "frequency":4400 }, "out":{ "frequency":4400 } }

has unambiguous paths 'in.frequency' and 'out.frequency', but the
shorthand 'frequency' could now match two separate places in the struct.

Now, let's look at what your testsuite changes say about this patch:

> @@ -271,6 +299,12 @@ main(int argc, char **argv)
>      add_test("/visitor/opts/i64/range/2big/full", &expect_fail,
>               "i64=-0x8000000000000000-0x7fffffffffffffff");
>  
> +    /* Test nested structs support */
> +    add_test("/visitor/opts/nested/unqualified", &expect_fail, "nint=13");

You are saying that an unqualified member no longer resolves (that is,
when nesting is in place, we MUST spell it by the long name);

> +    add_test("/visitor/opts/nested/both",        &expect_both,
> +             "sub0.nint=13,sub1.nint=17");
> +    add_test("/visitor/opts/nested/sub0",        &expect_sub0, "sub0.nint=13");
> +    add_test("/visitor/opts/nested/sub1",        &expect_sub1, "sub1.nint=13");

and that using qualified names gets at the nested struct members as
desired.  Furthermore, if we truly have converted ALL qapi unions to
flat unions, and if qapi unions were the ONLY reason that we had
flattening in the first place, and if we did NOT accept longhand
'data.c=1' for the flattened shorthand of 'c=1' within a simple union
branch, then it looks like you are 100% backwards-compatible.  But
that's quite a few things to verify.  Also, I'm not sure if your visitor
approaches the change of no longer flattening things in the most
efficient manner (I still haven't studied the rest of the patch closely).

I'd like to defer reviewing this until the qapi patch queue is not quite
so long, but the premise behind it is appealing.  However, I strongly
recommend that the commit message be improved to cover some of the
background that I spelled out here, as well as offering more proof than
just "hopefully", and an analysis of whether we are breaking any
longhand data.c=1 option spellings.
=?UTF-8?B?Wm9sdMOhbiBLxZF2w6Fnw7M=?= Sept. 17, 2015, 10:01 p.m. UTC | #2 
2015-09-17 23:24 keltezéssel, Eric Blake írta:
> On 09/07/2015 06:14 AM, Kővágó, Zoltán wrote:
>> The current OptsVisitor flattens the whole structure, if there are same
>> named fields under different paths (like `in' and `out' in `Audiodev'),
>> the current visitor can't cope with them (for example setting
>> `frequency=44100' will set the in's frequency to 44100 and leave out's
>> frequency unspecified).
>>
>> This patch fixes it, by always requiring a complete path in case of
>> nested structs.  Fields in the path are separated by dots, similar to C
>> structs (without pointers), like `in.frequency' or `out.frequency'.
>>
>> You must provide a full path even in non-ambiguous cases.  The qapi
>> flattening commits hopefully ensures that this change doesn't create
>> backward compatibility problems.
>
> It's the "hopefully" that worries me.
>
> If I understand correctly, prior to this series, we have several
> instances of qapi simple unions, where a member is a substruct when
> converting to QObject.  Conceptually:
>
> { 'struct':'Sub', 'data':{ 'c':'int' } }
> { 'union':'Main', 'data':{ 'X':'Sub' } }
>
> would accept the following QDict:
>
> { "type":"X", "data":{ "c":1 } }
>
> and directly translating that to QemuOpts would be spelled:
>
> -opt type=X,data.c=1
>
> but we wanted shorthand:
>
> -opt type=X,c=1
>
> So, the "flattening" that opts-visitor does is what allows "c" instead
> of "data.c" to refer to a nested member of a union.  It worked as long
> as 'c' appeared only once per visit of the overall qapi type (multiple
> branches of the union may have 'c', but for a given union branch, 'c'
> appears only once no matter what depth of substructs it is reached through).
>
> Question: do we actually ALLOW the user to specify data.c, or do we ONLY
> accept the shorthand?

The current implementation in qemu only allows the shorthand version, in 
fact it doesn't even care about the name of the parent struct/union. 
Neither Main nor Sub has a field names 'data.c', so the data.c=1 version 
is currently invalid.  But the problem is, as you noted, it only works 
if each field name is unique in the whole visited type (excluding 
non-visited union branches).

> This series then proceeds to teach opts-visitor how to handle flat
> unions.  Converting the above example to a flat union is done by:
>
> { 'enum':'Foo', 'data':['X'] }
> { 'struct':'Base', 'data':{ 'type':'Foo' } }
> { 'struct':'Sub', 'data':{ 'c':'int' } }
> { 'union':'Main', 'base':'Base', 'discriminator':'type',
>    'data':{ 'X':'Sub' } }
>
> which now accepts the following QDict:
>
> { "type":"X", "c":1" }
>
> (note the loss of the nested 'data' struct), and directly translating
> that to QemuOpts would be spelled:
>
> -opt type=X,c=1
>
> which exactly matches the shorthand that we previously accepted.  It
> completely gets rid of the data.c longhand, but I don't know if we
> accepted that in the first place.

No, data.c was never valid.

> So, the next question is whether we still need flattening.  If we no
> longer have any simple unions parsed by QemuOpts, then the flattening no
> longer helps us.  Furthermore, you have a case in audio where flattening
> hurts; the QDict:
>
> { "in":{ "frequency":4400 }, "out":{ "frequency":4400 } }
>
> has unambiguous paths 'in.frequency' and 'out.frequency', but the
> shorthand 'frequency' could now match two separate places in the struct.

This is why exactly the flattening needs to be moved from OptsVisitor to 
somewhere else.  If the qapi structs are flattened, old options (-net, 
-numa) continue to work as before, while at allows -audiodev to specify 
in.frequency and out.frequency.  Of course, this isn't the only possible 
approach, but my previous attempt where I modified OptsVisitor instead 
to allow both shorthand and full paths were also problematic:

http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg04189.html

>
> Now, let's look at what your testsuite changes say about this patch:
>
>> @@ -271,6 +299,12 @@ main(int argc, char **argv)
>>       add_test("/visitor/opts/i64/range/2big/full", &expect_fail,
>>                "i64=-0x8000000000000000-0x7fffffffffffffff");
>>
>> +    /* Test nested structs support */
>> +    add_test("/visitor/opts/nested/unqualified", &expect_fail, "nint=13");
>
> You are saying that an unqualified member no longer resolves (that is,
> when nesting is in place, we MUST spell it by the long name);
>
>> +    add_test("/visitor/opts/nested/both",        &expect_both,
>> +             "sub0.nint=13,sub1.nint=17");
>> +    add_test("/visitor/opts/nested/sub0",        &expect_sub0, "sub0.nint=13");
>> +    add_test("/visitor/opts/nested/sub1",        &expect_sub1, "sub1.nint=13");
>
> and that using qualified names gets at the nested struct members as
> desired.  Furthermore, if we truly have converted ALL qapi unions to
> flat unions, and if qapi unions were the ONLY reason that we had
> flattening in the first place, and if we did NOT accept longhand
> 'data.c=1' for the flattened shorthand of 'c=1' within a simple union
> branch, then it looks like you are 100% backwards-compatible.  But
> that's quite a few things to verify.  Also, I'm not sure if your visitor
> approaches the change of no longer flattening things in the most
> efficient manner (I still haven't studied the rest of the patch closely).
>
> I'd like to defer reviewing this until the qapi patch queue is not quite
> so long, but the premise behind it is appealing.  However, I strongly
> recommend that the commit message be improved to cover some of the
> background that I spelled out here, as well as offering more proof than
> just "hopefully", and an analysis of whether we are breaking any
> longhand data.c=1 option spellings.
>
diff mbox

Patch

diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index aa68814..2c2710b 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -71,6 +71,7 @@  struct OptsVisitor
      * schema, with a single mandatory scalar member. */
     ListMode list_mode;
     GQueue *repeated_opts;
+    char *repeated_name;
 
     /* When parsing a list of repeating options as integers, values of the form
      * "a-b", representing a closed interval, are allowed. Elements in the
@@ -86,6 +87,9 @@  struct OptsVisitor
      * not survive or escape the OptsVisitor object.
      */
     QemuOpt *fake_id_opt;
+
+    /* List of field names leading to the current structure. */
+    GQueue *nested_names;
 };
 
 
@@ -100,6 +104,7 @@  static void
 opts_visitor_insert(GHashTable *unprocessed_opts, const QemuOpt *opt)
 {
     GQueue *list;
+    assert(opt);
 
     list = g_hash_table_lookup(unprocessed_opts, opt->name);
     if (list == NULL) {
@@ -127,6 +132,9 @@  opts_start_struct(Visitor *v, void **obj, const char *kind,
     if (obj) {
         *obj = g_malloc0(size > 0 ? size : 1);
     }
+
+    g_queue_push_tail(ov->nested_names, (gpointer) name);
+
     if (ov->depth++ > 0) {
         return;
     }
@@ -169,6 +177,8 @@  opts_end_struct(Visitor *v, Error **errp)
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
     GQueue *any;
 
+    g_queue_pop_tail(ov->nested_names);
+
     if (--ov->depth > 0) {
         return;
     }
@@ -198,15 +208,55 @@  opts_end_implicit_struct(Visitor *v, Error **errp)
 }
 
 
+static void
+sum_strlen(gpointer data, gpointer user_data)
+{
+    const char *str = data;
+    size_t *sum_len = user_data;
+
+    if (str) { /* skip NULLs */
+        *sum_len += strlen(str) + 1;
+    }
+}
+
+static void
+append_str(gpointer data, gpointer user_data)
+{
+    const char *str = data;
+    char *concat_str = user_data;
+
+    if (str) {
+        strcat(concat_str, str);
+        strcat(concat_str, ".");
+    }
+}
+
+/* lookup a name, using a fully qualified version */
 static GQueue *
-lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp)
+lookup_distinct(const OptsVisitor *ov, const char *name, char **out_key,
+                Error **errp)
 {
-    GQueue *list;
+    GQueue *list = NULL;
+    char *key;
+    size_t sum_len = strlen(name);
+
+    g_queue_foreach(ov->nested_names, sum_strlen, &sum_len);
+    key = g_malloc(sum_len+1);
+    key[0] = 0;
+    g_queue_foreach(ov->nested_names, append_str, key);
+    strcat(key, name);
+
+    list = g_hash_table_lookup(ov->unprocessed_opts, key);
+    if (list && out_key) {
+        *out_key = key;
+        key = NULL;
+    }
 
-    list = g_hash_table_lookup(ov->unprocessed_opts, name);
     if (!list) {
         error_setg(errp, QERR_MISSING_PARAMETER, name);
     }
+
+    g_free(key);
     return list;
 }
 
@@ -218,7 +268,8 @@  opts_start_list(Visitor *v, const char *name, Error **errp)
 
     /* we can't traverse a list in a list */
     assert(ov->list_mode == LM_NONE);
-    ov->repeated_opts = lookup_distinct(ov, name, errp);
+    assert(ov->repeated_opts == NULL && ov->repeated_name == NULL);
+    ov->repeated_opts = lookup_distinct(ov, name, &ov->repeated_name, errp);
     if (ov->repeated_opts != NULL) {
         ov->list_mode = LM_STARTED;
     }
@@ -254,11 +305,9 @@  opts_next_list(Visitor *v, GenericList **list, Error **errp)
         /* range has been completed, fall through in order to pop option */
 
     case LM_IN_PROGRESS: {
-        const QemuOpt *opt;
-
-        opt = g_queue_pop_head(ov->repeated_opts);
+        g_queue_pop_head(ov->repeated_opts);
         if (g_queue_is_empty(ov->repeated_opts)) {
-            g_hash_table_remove(ov->unprocessed_opts, opt->name);
+            g_hash_table_remove(ov->unprocessed_opts, ov->repeated_name);
             return NULL;
         }
         link = &(*list)->next;
@@ -284,22 +333,28 @@  opts_end_list(Visitor *v, Error **errp)
            ov->list_mode == LM_SIGNED_INTERVAL ||
            ov->list_mode == LM_UNSIGNED_INTERVAL);
     ov->repeated_opts = NULL;
+
+    g_free(ov->repeated_name);
+    ov->repeated_name = NULL;
+
     ov->list_mode = LM_NONE;
 }
 
 
 static const QemuOpt *
-lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp)
+lookup_scalar(const OptsVisitor *ov, const char *name, char** out_key,
+              Error **errp)
 {
     if (ov->list_mode == LM_NONE) {
         GQueue *list;
 
         /* the last occurrence of any QemuOpt takes effect when queried by name
          */
-        list = lookup_distinct(ov, name, errp);
+        list = lookup_distinct(ov, name, out_key, errp);
         return list ? g_queue_peek_tail(list) : NULL;
     }
     assert(ov->list_mode == LM_IN_PROGRESS);
+    assert(out_key == NULL || *out_key == NULL);
     return g_queue_peek_head(ov->repeated_opts);
 }
 
@@ -321,13 +376,15 @@  opts_type_str(Visitor *v, char **obj, const char *name, Error **errp)
 {
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
     const QemuOpt *opt;
+    char *key = NULL;
 
-    opt = lookup_scalar(ov, name, errp);
+    opt = lookup_scalar(ov, name, &key, errp);
     if (!opt) {
         return;
     }
     *obj = g_strdup(opt->str ? opt->str : "");
-    processed(ov, name);
+    processed(ov, key);
+    g_free(key);
 }
 
 
@@ -337,8 +394,9 @@  opts_type_bool(Visitor *v, bool *obj, const char *name, Error **errp)
 {
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
     const QemuOpt *opt;
+    char *key = NULL;
 
-    opt = lookup_scalar(ov, name, errp);
+    opt = lookup_scalar(ov, name, &key, errp);
     if (!opt) {
         return;
     }
@@ -355,13 +413,15 @@  opts_type_bool(Visitor *v, bool *obj, const char *name, Error **errp)
         } else {
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
                        "on|yes|y|off|no|n");
+            g_free(key);
             return;
         }
     } else {
         *obj = true;
     }
 
-    processed(ov, name);
+    processed(ov, key);
+    g_free(key);
 }
 
 
@@ -373,13 +433,14 @@  opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
     const char *str;
     long long val;
     char *endptr;
+    char *key = NULL;
 
     if (ov->list_mode == LM_SIGNED_INTERVAL) {
         *obj = ov->range_next.s;
         return;
     }
 
-    opt = lookup_scalar(ov, name, errp);
+    opt = lookup_scalar(ov, name, &key, errp);
     if (!opt) {
         return;
     }
@@ -393,11 +454,13 @@  opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
     if (errno == 0 && endptr > str && INT64_MIN <= val && val <= INT64_MAX) {
         if (*endptr == '\0') {
             *obj = val;
-            processed(ov, name);
+            processed(ov, key);
+            g_free(key);
             return;
         }
         if (*endptr == '-' && ov->list_mode == LM_IN_PROGRESS) {
             long long val2;
+            assert(key == NULL);
 
             str = endptr + 1;
             val2 = strtoll(str, &endptr, 0);
@@ -418,6 +481,7 @@  opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
     error_setg(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
                (ov->list_mode == LM_NONE) ? "an int64 value" :
                                             "an int64 value or range");
+    g_free(key);
 }
 
 
@@ -429,13 +493,14 @@  opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
     const char *str;
     unsigned long long val;
     char *endptr;
+    char *key = NULL;
 
     if (ov->list_mode == LM_UNSIGNED_INTERVAL) {
         *obj = ov->range_next.u;
         return;
     }
 
-    opt = lookup_scalar(ov, name, errp);
+    opt = lookup_scalar(ov, name, &key, errp);
     if (!opt) {
         return;
     }
@@ -447,11 +512,13 @@  opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
     if (parse_uint(str, &val, &endptr, 0) == 0 && val <= UINT64_MAX) {
         if (*endptr == '\0') {
             *obj = val;
-            processed(ov, name);
+            processed(ov, key);
+            g_free(key);
             return;
         }
         if (*endptr == '-' && ov->list_mode == LM_IN_PROGRESS) {
             unsigned long long val2;
+            assert(key == NULL);
 
             str = endptr + 1;
             if (parse_uint_full(str, &val2, 0) == 0 &&
@@ -470,6 +537,7 @@  opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
     error_setg(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
                (ov->list_mode == LM_NONE) ? "a uint64 value" :
                                             "a uint64 value or range");
+    g_free(key);
 }
 
 
@@ -480,8 +548,9 @@  opts_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
     const QemuOpt *opt;
     int64_t val;
     char *endptr;
+    char *key = NULL;
 
-    opt = lookup_scalar(ov, name, errp);
+    opt = lookup_scalar(ov, name, &key, errp);
     if (!opt) {
         return;
     }
@@ -491,11 +560,13 @@  opts_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
     if (val < 0 || *endptr) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
                    "a size value representible as a non-negative int64");
+        g_free(key);
         return;
     }
 
     *obj = val;
-    processed(ov, name);
+    processed(ov, key);
+    g_free(key);
 }
 
 
@@ -506,7 +577,7 @@  opts_optional(Visitor *v, bool *present, const char *name, Error **errp)
 
     /* we only support a single mandatory scalar field in a list node */
     assert(ov->list_mode == LM_NONE);
-    *present = (lookup_distinct(ov, name, NULL) != NULL);
+    *present = (lookup_distinct(ov, name, NULL, NULL) != NULL);
 }
 
 
@@ -517,6 +588,8 @@  opts_visitor_new(const QemuOpts *opts)
 
     ov = g_malloc0(sizeof *ov);
 
+    ov->nested_names = g_queue_new();
+
     ov->visitor.start_struct = &opts_start_struct;
     ov->visitor.end_struct   = &opts_end_struct;
 
@@ -560,6 +633,7 @@  opts_visitor_cleanup(OptsVisitor *ov)
     if (ov->unprocessed_opts != NULL) {
         g_hash_table_destroy(ov->unprocessed_opts);
     }
+    g_queue_free(ov->nested_names);
     g_free(ov->fake_id_opt);
     g_free(ov);
 }
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index a9e5aab..c4ab8b9 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -87,6 +87,11 @@ 
 { 'command': 'user_def_cmd3', 'data': {'a': 'int', '*b': 'int' },
   'returns': 'int' }
 
+# For testing hierarchy support in opts-visitor
+{ 'struct': 'UserDefOptionsSub',
+  'data': {
+    '*nint': 'int' } }
+
 # For testing integer range flattening in opts-visitor. The following schema
 # corresponds to the option format:
 #
@@ -100,7 +105,9 @@ 
     '*u64' : [ 'uint64' ],
     '*u16' : [ 'uint16' ],
     '*i64x':   'int'     ,
-    '*u64x':   'uint64'  } }
+    '*u64x':   'uint64'  ,
+    'sub0':    'UserDefOptionsSub',
+    'sub1':    'UserDefOptionsSub' } }
 
 # testing event
 { 'struct': 'EventStructOne',
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index b0b7187..0e8baca 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -17,7 +17,8 @@ 
  OrderedDict([('command', 'user_def_cmd1'), ('data', OrderedDict([('ud1a', 'UserDefOne')]))]),
  OrderedDict([('command', 'user_def_cmd2'), ('data', OrderedDict([('ud1a', 'UserDefOne'), ('*ud1b', 'UserDefOne')])), ('returns', 'UserDefTwo')]),
  OrderedDict([('command', 'user_def_cmd3'), ('data', OrderedDict([('a', 'int'), ('*b', 'int')])), ('returns', 'int')]),
- OrderedDict([('struct', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))]),
+ OrderedDict([('struct', 'UserDefOptionsSub'), ('data', OrderedDict([('*nint', 'int')]))]),
+ OrderedDict([('struct', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64'), ('sub0', 'UserDefOptionsSub'), ('sub1', 'UserDefOptionsSub')]))]),
  OrderedDict([('struct', 'EventStructOne'), ('data', OrderedDict([('struct1', 'UserDefOne'), ('string', 'str'), ('*enum2', 'EnumOne')]))]),
  OrderedDict([('event', 'EVENT_A')]),
  OrderedDict([('event', 'EVENT_B'), ('data', OrderedDict())]),
@@ -48,7 +49,8 @@ 
  OrderedDict([('struct', 'UserDefB'), ('data', OrderedDict([('intb', 'int')]))]),
  OrderedDict([('struct', 'UserDefUnionBase'), ('base', 'UserDefZero'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
  OrderedDict([('struct', 'UserDefC'), ('data', OrderedDict([('string1', 'str'), ('string2', 'str')]))]),
- OrderedDict([('struct', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))]),
+ OrderedDict([('struct', 'UserDefOptionsSub'), ('data', OrderedDict([('*nint', 'int')]))]),
+ OrderedDict([('struct', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64'), ('sub0', 'UserDefOptionsSub'), ('sub1', 'UserDefOptionsSub')]))]),
  OrderedDict([('struct', 'EventStructOne'), ('data', OrderedDict([('struct1', 'UserDefOne'), ('string', 'str'), ('*enum2', 'EnumOne')]))]),
  OrderedDict([('struct', '__org.qemu_x-Base'), ('data', OrderedDict([('__org.qemu_x-member1', '__org.qemu_x-Enum')]))]),
  OrderedDict([('struct', '__org.qemu_x-Struct'), ('base', '__org.qemu_x-Base'), ('data', OrderedDict([('__org.qemu_x-member2', 'str')]))]),
diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c
index 1c753d9..4393266 100644
--- a/tests/test-opts-visitor.c
+++ b/tests/test-opts-visitor.c
@@ -178,6 +178,34 @@  expect_u64_max(OptsVisitorFixture *f, gconstpointer test_data)
     g_assert(f->userdef->u64->value == UINT64_MAX);
 }
 
+static void
+expect_both(OptsVisitorFixture *f, gconstpointer test_data)
+{
+    expect_ok(f, test_data);
+    g_assert(f->userdef->sub0->has_nint);
+    g_assert(f->userdef->sub0->nint == 13);
+    g_assert(f->userdef->sub1->has_nint);
+    g_assert(f->userdef->sub1->nint == 17);
+}
+
+static void
+expect_sub0(OptsVisitorFixture *f, gconstpointer test_data)
+{
+    expect_ok(f, test_data);
+    g_assert(f->userdef->sub0->has_nint);
+    g_assert(f->userdef->sub0->nint == 13);
+    g_assert(!f->userdef->sub1->has_nint);
+}
+
+static void
+expect_sub1(OptsVisitorFixture *f, gconstpointer test_data)
+{
+    expect_ok(f, test_data);
+    g_assert(!f->userdef->sub0->has_nint);
+    g_assert(f->userdef->sub1->has_nint);
+    g_assert(f->userdef->sub1->nint == 13);
+}
+
 /* test cases */
 
 int
@@ -271,6 +299,12 @@  main(int argc, char **argv)
     add_test("/visitor/opts/i64/range/2big/full", &expect_fail,
              "i64=-0x8000000000000000-0x7fffffffffffffff");
 
+    /* Test nested structs support */
+    add_test("/visitor/opts/nested/unqualified", &expect_fail, "nint=13");
+    add_test("/visitor/opts/nested/both",        &expect_both,
+             "sub0.nint=13,sub1.nint=17");
+    add_test("/visitor/opts/nested/sub0",        &expect_sub0, "sub0.nint=13");
+    add_test("/visitor/opts/nested/sub1",        &expect_sub1, "sub1.nint=13");
     g_test_run();
     return 0;
 }