From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43087) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5dPY-0006Nz-Vz for qemu-devel@nongnu.org; Thu, 18 Jun 2015 13:15:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z5dPO-0001zU-VU for qemu-devel@nongnu.org; Thu, 18 Jun 2015 13:15:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33664) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5dPO-0001we-KP for qemu-devel@nongnu.org; Thu, 18 Jun 2015 13:15:22 -0400 Message-ID: <5582FCA6.90205@redhat.com> Date: Thu, 18 Jun 2015 19:15:18 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 5/8] qapi: support nested structs in OptsVisitor List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?IkvFkXbDoWfDsywgWm9sdMOhbiI=?= Cc: Michael Roth , Markus Armbruster , qemu-devel@nongnu.org, Gerd Hoffmann On 06/18/15 18:43, K=C5=91v=C3=A1g=C3=B3, Zolt=C3=A1n 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=3D44100' will set the in's frequency to 44100 and leave out'= s > frequency unspecified). >=20 > 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'. >=20 > You must provide a full path even in non-ambigous cases. The previous > two commits hopefully ensures that this change doesn't create backward > compatibility problems. >=20 > Signed-off-by: K=C5=91v=C3=A1g=C3=B3, Zolt=C3=A1n >=20 > --- >=20 > Change from v2: > * only fully qualified paths are allowed >=20 > qapi/opts-visitor.c | 114 ++++++++++++++++++++++++= ++------ > tests/qapi-schema/qapi-schema-test.json | 9 ++- > tests/test-opts-visitor.c | 34 ++++++++++ > 3 files changed, 135 insertions(+), 22 deletions(-) >=20 > diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c > index f02059d..7a80442 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; > =20 > /* 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; > }; > =20 > =20 > @@ -100,6 +104,7 @@ static void > opts_visitor_insert(GHashTable *unprocessed_opts, const QemuOpt *opt) > { > GQueue *list; > + assert(opt); > =20 > list =3D g_hash_table_lookup(unprocessed_opts, opt->name); > if (list =3D=3D NULL) { > @@ -127,6 +132,9 @@ opts_start_struct(Visitor *v, void **obj, const cha= r *kind, > if (obj) { > *obj =3D 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 =3D DO_UPCAST(OptsVisitor, visitor, v); > GQueue *any; > =20 > + g_queue_pop_tail(ov->nested_names); > + > if (--ov->depth > 0) { > return; > } > @@ -198,15 +208,54 @@ opts_end_implicit_struct(Visitor *v, Error **errp= ) > } > =20 > =20 > +static void > +sum_strlen(gpointer data, gpointer user_data) > +{ > + const char *str =3D data; > + size_t *sum_len =3D user_data; > + > + if (str) { /* skip NULLs */ > + *sum_len +=3D strlen(str) + 1; > + } > +} > + > +static void > +append_str(gpointer data, gpointer user_data) > +{ > + const char *str =3D data; > + char *concat_str =3D 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_ke= y, > + Error **errp) > { > - GQueue *list; > + GQueue *list =3D NULL; > + char *key; > + size_t sum_len =3D strlen(name); > + > + g_queue_foreach(ov->nested_names, sum_strlen, &sum_len); > + key =3D g_malloc(sum_len+1); > + key[0] =3D 0; > + g_queue_foreach(ov->nested_names, append_str, key); > + strcat(key, name); > + > + list =3D g_hash_table_lookup(ov->unprocessed_opts, key); > + if (list && out_key) { > + *out_key =3D g_strdup(key); > + } > =20 > - list =3D g_hash_table_lookup(ov->unprocessed_opts, name); > if (!list) { > error_set(errp, QERR_MISSING_PARAMETER, name); > } > + > + g_free(key); > return list; > } > =20 > @@ -218,7 +267,7 @@ opts_start_list(Visitor *v, const char *name, Error= **errp) > =20 > /* we can't traverse a list in a list */ > assert(ov->list_mode =3D=3D LM_NONE); > - ov->repeated_opts =3D lookup_distinct(ov, name, errp); > + ov->repeated_opts =3D lookup_distinct(ov, name, &ov->repeated_name= , errp); > if (ov->repeated_opts !=3D NULL) { > ov->list_mode =3D LM_STARTED; > } > @@ -254,11 +303,9 @@ opts_next_list(Visitor *v, GenericList **list, Err= or **errp) > /* range has been completed, fall through in order to pop opti= on */ > =20 > case LM_IN_PROGRESS: { > - const QemuOpt *opt; > - > - opt =3D 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_nam= e); > return NULL; > } > link =3D &(*list)->next; > @@ -284,22 +331,28 @@ opts_end_list(Visitor *v, Error **errp) > ov->list_mode =3D=3D LM_SIGNED_INTERVAL || > ov->list_mode =3D=3D LM_UNSIGNED_INTERVAL); > ov->repeated_opts =3D NULL; > + > + g_free(ov->repeated_name); > + ov->repeated_name =3D NULL; > + > ov->list_mode =3D LM_NONE; > } > =20 > =20 > 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 =3D=3D LM_NONE) { > GQueue *list; > =20 > /* the last occurrence of any QemuOpt takes effect when querie= d by name > */ > - list =3D lookup_distinct(ov, name, errp); > + list =3D lookup_distinct(ov, name, out_key, errp); > return list ? g_queue_peek_tail(list) : NULL; > } > assert(ov->list_mode =3D=3D LM_IN_PROGRESS); > + assert(out_key =3D=3D NULL || *out_key =3D=3D NULL); > return g_queue_peek_head(ov->repeated_opts); > } > =20 > @@ -321,13 +374,15 @@ opts_type_str(Visitor *v, char **obj, const char = *name, Error **errp) > { > OptsVisitor *ov =3D DO_UPCAST(OptsVisitor, visitor, v); > const QemuOpt *opt; > + char *key =3D NULL; > =20 > - opt =3D lookup_scalar(ov, name, errp); > + opt =3D lookup_scalar(ov, name, &key, errp); > if (!opt) { > return; > } > *obj =3D g_strdup(opt->str ? opt->str : ""); > - processed(ov, name); > + processed(ov, key); > + g_free(key); > } > =20 > =20 > @@ -337,8 +392,9 @@ opts_type_bool(Visitor *v, bool *obj, const char *n= ame, Error **errp) > { > OptsVisitor *ov =3D DO_UPCAST(OptsVisitor, visitor, v); > const QemuOpt *opt; > + char *key =3D NULL; > =20 > - opt =3D lookup_scalar(ov, name, errp); > + opt =3D lookup_scalar(ov, name, &key, errp); > if (!opt) { > return; > } > @@ -355,13 +411,15 @@ opts_type_bool(Visitor *v, bool *obj, const char = *name, Error **errp) > } else { > error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name, > "on|yes|y|off|no|n"); > + g_free(key); > return; > } > } else { > *obj =3D true; > } > =20 > - processed(ov, name); > + processed(ov, key); > + g_free(key); > } > =20 > =20 > @@ -373,13 +431,14 @@ opts_type_int(Visitor *v, int64_t *obj, const cha= r *name, Error **errp) > const char *str; > long long val; > char *endptr; > + char *key =3D NULL; > =20 > if (ov->list_mode =3D=3D LM_SIGNED_INTERVAL) { > *obj =3D ov->range_next.s; > return; > } > =20 > - opt =3D lookup_scalar(ov, name, errp); > + opt =3D lookup_scalar(ov, name, &key, errp); > if (!opt) { > return; > } > @@ -393,11 +452,13 @@ opts_type_int(Visitor *v, int64_t *obj, const cha= r *name, Error **errp) > if (errno =3D=3D 0 && endptr > str && INT64_MIN <=3D val && val <=3D= INT64_MAX) { > if (*endptr =3D=3D '\0') { > *obj =3D val; > - processed(ov, name); > + processed(ov, key); > + g_free(key); > return; > } > if (*endptr =3D=3D '-' && ov->list_mode =3D=3D LM_IN_PROGRESS)= { > long long val2; > + assert(key =3D=3D NULL); > =20 > str =3D endptr + 1; > val2 =3D strtoll(str, &endptr, 0); > @@ -418,6 +479,7 @@ opts_type_int(Visitor *v, int64_t *obj, const char = *name, Error **errp) > error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name, > (ov->list_mode =3D=3D LM_NONE) ? "an int64 value" : > "an int64 value or range"); > + g_free(key); > } > =20 > =20 > @@ -429,13 +491,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 =3D NULL; > =20 > if (ov->list_mode =3D=3D LM_UNSIGNED_INTERVAL) { > *obj =3D ov->range_next.u; > return; > } > =20 > - opt =3D lookup_scalar(ov, name, errp); > + opt =3D lookup_scalar(ov, name, &key, errp); > if (!opt) { > return; > } > @@ -447,11 +510,13 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const= char *name, Error **errp) > if (parse_uint(str, &val, &endptr, 0) =3D=3D 0 && val <=3D UINT64_= MAX) { > if (*endptr =3D=3D '\0') { > *obj =3D val; > - processed(ov, name); > + processed(ov, key); > + g_free(key); > return; > } > if (*endptr =3D=3D '-' && ov->list_mode =3D=3D LM_IN_PROGRESS)= { > unsigned long long val2; > + assert(key =3D=3D NULL); > =20 > str =3D endptr + 1; > if (parse_uint_full(str, &val2, 0) =3D=3D 0 && > @@ -470,6 +535,7 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const c= har *name, Error **errp) > error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name, > (ov->list_mode =3D=3D LM_NONE) ? "a uint64 value" : > "a uint64 value or range"); > + g_free(key); > } > =20 > =20 > @@ -480,8 +546,9 @@ opts_type_size(Visitor *v, uint64_t *obj, const cha= r *name, Error **errp) > const QemuOpt *opt; > int64_t val; > char *endptr; > + char *key =3D NULL; > =20 > - opt =3D lookup_scalar(ov, name, errp); > + opt =3D lookup_scalar(ov, name, &key, errp); > if (!opt) { > return; > } > @@ -491,11 +558,13 @@ opts_type_size(Visitor *v, uint64_t *obj, const c= har *name, Error **errp) > if (val < 0 || *endptr) { > error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name, > "a size value representible as a non-negative int64"= ); > + g_free(key); > return; > } > =20 > *obj =3D val; > - processed(ov, name); > + processed(ov, key); > + g_free(key); > } > =20 > =20 > @@ -506,7 +575,7 @@ opts_optional(Visitor *v, bool *present, const char= *name, Error **errp) > =20 > /* we only support a single mandatory scalar field in a list node = */ > assert(ov->list_mode =3D=3D LM_NONE); > - *present =3D (lookup_distinct(ov, name, NULL) !=3D NULL); > + *present =3D (lookup_distinct(ov, name, NULL, NULL) !=3D NULL); > } > =20 > =20 > @@ -517,6 +586,8 @@ opts_visitor_new(const QemuOpts *opts) > =20 > ov =3D g_malloc0(sizeof *ov); > =20 > + ov->nested_names =3D g_queue_new(); > + > ov->visitor.start_struct =3D &opts_start_struct; > ov->visitor.end_struct =3D &opts_end_struct; > =20 > @@ -560,6 +631,7 @@ opts_visitor_cleanup(OptsVisitor *ov) > if (ov->unprocessed_opts !=3D 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-schem= a/qapi-schema-test.json > index c7eaa86..a818eff 100644 > --- a/tests/qapi-schema/qapi-schema-test.json > +++ b/tests/qapi-schema/qapi-schema-test.json > @@ -81,6 +81,11 @@ > { 'command': 'user_def_cmd3', 'data': {'a': 'int', '*b': 'int' }, > 'returns': 'int' } > =20 > +# 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: > # > @@ -94,7 +99,9 @@ > '*u64' : [ 'uint64' ], > '*u16' : [ 'uint16' ], > '*i64x': 'int' , > - '*u64x': 'uint64' } } > + '*u64x': 'uint64' , > + 'sub0': 'UserDefOptionsSub', > + 'sub1': 'UserDefOptionsSub' } } > =20 > # testing event > { 'struct': 'EventStructOne', > diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c > index ebeee5d..9199141 100644 > --- a/tests/test-opts-visitor.c > +++ b/tests/test-opts-visitor.c > @@ -177,6 +177,34 @@ expect_u64_max(OptsVisitorFixture *f, gconstpointe= r test_data) > g_assert(f->userdef->u64->value =3D=3D UINT64_MAX); > } > =20 > +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 =3D=3D 13); > + g_assert(f->userdef->sub1->has_nint); > + g_assert(f->userdef->sub1->nint =3D=3D 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 =3D=3D 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 =3D=3D 13); > +} > + > /* test cases */ > =20 > int > @@ -270,6 +298,12 @@ main(int argc, char **argv) > add_test("/visitor/opts/i64/range/2big/full", &expect_fail, > "i64=3D-0x8000000000000000-0x7fffffffffffffff"); > =20 > + /* Test nested structs support */ > + add_test("/visitor/opts/nested/unqualified", &expect_fail, "nint=3D= 13"); > + add_test("/visitor/opts/nested/both", &expect_both, > + "sub0.nint=3D13,sub1.nint=3D17"); > + add_test("/visitor/opts/nested/sub0", &expect_sub0, "sub0.n= int=3D13"); > + add_test("/visitor/opts/nested/sub1", &expect_sub1, "sub1.n= int=3D13"); > g_test_run(); > return 0; > } >=20 Re patch v3 1/8 -- apparently I was not around when the start_implicit_struct() and end_implicit_struct() visitor callbacks were introduced, so I don't know what they are good for, but on the surface, that patch might even be correct. I can't tell. Re patch v3 5/8, ie. this patch -- if there has been some kind of "master blurb" for this feature, ie. justification for nested structs, I'm unaware of it. (I'm not getting qemu-devel email, just personal Cc's.) Can you please send me a link into the archive where this feature is justifed / discussed? I have one related, and one independent note here: (1) The related note -- OptsVisitor was introduced in commit eb7ee2cbeb. You can see in the commit message that "union of flat structures" was a foundational design trait for OptsVisitor, so if you want nested structs, it's not a "bug to fix", but a new feature to add. (Namely, refer to the allowed members of "struct for discriminator case 1" -- they can only be scalar members, or restricted format lists for repeating optargs. Structure nesting is not allowed on any level.) The objective was to support then-existent command lines (ie. parse options into QAPI schemas). As far as I (vaguely) recall, Eric has since (independently) flattened a bunch of QAPI structs as well. So I'm unsure if and why nested structs are "again" a "thing". (In any case this should not be taken as my opposition; I'm just uninformed. That link I mentioned above could inform me.) (2) The independent note. OptsVisitor is one of the hardest-to-understand things I've implemented. It has always been hard to understand for myself. As far as I recall, I struggled in my mind to cover all corner cases and prove stuff right. Later I was "asked" to add range support to it (ie. turn a range like 3-7 into a list of integers 3, 4, 5, 6, 7) and I was sweating bloody bullets to avoid regressions. So, this code requires careful tiptoeing. I do not want to discourage changes to it (and I certainly encourage reviewers to look at your patches). I'm just saying I personally can't be your reviewer. I lost this context long ago, I dread looking at this code again, and I don't feel safe saying either yay or nay. You could *almost* accuse me of having thrown this code over the fence. My only excuse (a good one though) is that I only wrote this code because I was asked to. (If you're familiar with the Bioshock universe, I heard "would you kindly".) I'm told that this code has proved somewhat useful, which I'm glad about, but I really can't support it. Sorry about that. So, I'm neither acking nor nacking; don't wait for my feedback. (Apologies for the wall of text, but I guess it's better to say "sorry can't help you" in too many words than not reacting at all.) Thanks L=C3=A1szl=C3=B3 (since we're using Hungarian accents in this thread :))