From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56172) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5doc-0000rw-J4 for qemu-devel@nongnu.org; Thu, 18 Jun 2015 13:41:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z5doZ-0000Zy-MK for qemu-devel@nongnu.org; Thu, 18 Jun 2015 13:41:26 -0400 Received: from mail-wi0-x22a.google.com ([2a00:1450:400c:c05::22a]:33823) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5doZ-0000Zu-74 for qemu-devel@nongnu.org; Thu, 18 Jun 2015 13:41:23 -0400 Received: by wicnd19 with SMTP id nd19so29822902wic.1 for ; Thu, 18 Jun 2015 10:41:22 -0700 (PDT) From: "=?UTF-8?B?S8WRdsOhZ8OzIFpvbHTDoW4=?=" References: <5582FCA6.90205@redhat.com> Message-ID: <55830170.2040403@gmail.com> Date: Thu, 18 Jun 2015 19:35:44 +0200 MIME-Version: 1.0 In-Reply-To: <5582FCA6.90205@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit 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: Laszlo Ersek Cc: Michael Roth , Markus Armbruster , qemu-devel@nongnu.org, Gerd Hoffmann 2015-06-18 19:15 keltezéssel, Laszlo Ersek írta: > On 06/18/15 18:43, 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-ambigous cases. The previous >> two commits hopefully ensures that this change doesn't create backward >> compatibility problems. >> >> Signed-off-by: Kővágó, Zoltán >> >> --- >> >> Change from v2: >> * only fully qualified paths are allowed >> >> 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(-) >> >> 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; >> >> /* 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,54 @@ 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 = g_strdup(key); >> + } >> >> - list = g_hash_table_lookup(ov->unprocessed_opts, name); >> if (!list) { >> error_set(errp, QERR_MISSING_PARAMETER, name); >> } >> + >> + g_free(key); >> return list; >> } >> >> @@ -218,7 +267,7 @@ 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); >> + ov->repeated_opts = lookup_distinct(ov, name, &ov->repeated_name, errp); >> if (ov->repeated_opts != NULL) { >> ov->list_mode = LM_STARTED; >> } >> @@ -254,11 +303,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 +331,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 +374,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 +392,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 +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 = true; >> } >> >> - processed(ov, name); >> + processed(ov, key); >> + g_free(key); >> } >> >> >> @@ -373,13 +431,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 +452,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 +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 == LM_NONE) ? "an int64 value" : >> "an int64 value or range"); >> + g_free(key); >> } >> >> >> @@ -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 = 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 +510,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 +535,7 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp) >> error_set(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 +546,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 +558,13 @@ opts_type_size(Visitor *v, uint64_t *obj, const char *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; >> } >> >> *obj = val; >> - processed(ov, name); >> + processed(ov, key); >> + g_free(key); >> } >> >> >> @@ -506,7 +575,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 +586,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 +631,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 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' } >> >> +# 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' } } >> >> # 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, 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 >> @@ -270,6 +298,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; >> } >> > > 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. Actually I'm not completely sure that the visitor code generated are correct in case of implicit structs. In case of normal structs, there's an if (*obj) { ... } check, but in case of implicit structs it just visits the fields without checking (which is problematic, since the default visit_start_implicit_struct is just a no-op, causing segfaults...) > 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? This is needed for my -audiodev option, see v2 patch thread: http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg04189.html And probably this as an example where it's useful: http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg04186.html > 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.) Check my second link, it contains a good example where it's useful (to specify input and output audio settings independently, like in.frequency and out.frequency. Of course we could do like in-frequency and out-frequency and a bunch of copy paste, but this is a much better way imho, even if it requires some changes to how existing things work). > (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.) All right, no problem. > > Thanks > László (since we're using Hungarian accents in this thread :)) > Thanks Zoltán :)