[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 5/8] qapi: support nested structs in OptsVisi
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH v3 5/8] qapi: support nested structs in OptsVisitor |
Date: |
Thu, 18 Jun 2015 19:15:18 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 |
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 <address@hidden>
>
> ---
>
> 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.
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ászló (since we're using Hungarian accents in this thread :))
- [Qemu-devel] [PATCH v3 0/8] -audiodev option, Kővágó, Zoltán, 2015/06/18
- [Qemu-devel] [PATCH v3 2/8] qapi: convert NumaOptions into a flat union, Kővágó, Zoltán, 2015/06/18
- [Qemu-devel] [PATCH v3 4/8] qapi: qapi for audio backends, Kővágó, Zoltán, 2015/06/18
- [Qemu-devel] [PATCH v3 1/8] qapi: support implicit structs in OptsVisitor, Kővágó, Zoltán, 2015/06/18
- [Qemu-devel] [PATCH v3 6/8] opts: produce valid command line in qemu_opts_print, Kővágó, Zoltán, 2015/06/18
- [Qemu-devel] [PATCH v3 5/8] qapi: support nested structs in OptsVisitor, Kővágó, Zoltán, 2015/06/18
- Re: [Qemu-devel] [PATCH v3 5/8] qapi: support nested structs in OptsVisitor,
Laszlo Ersek <=
- [Qemu-devel] [PATCH v3 7/8] audio: use qapi AudioFormat instead of audfmt_e, Kővágó, Zoltán, 2015/06/18
- [Qemu-devel] [PATCH v3 3/8] qapi: change Netdev and NetLegacy into a flat union, Kővágó, Zoltán, 2015/06/18
- [Qemu-devel] [PATCH v3 8/8] audio: -audiodev command line option, Kővágó, Zoltán, 2015/06/18