Hi
On Thu, Aug 6, 2015 at 8:28 PM, Kővágó, Zoltán <dirty.ice...@gmail.com> 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 qapi
> flattening commits hopefully ensures that this change doesn't create
> backward compatibility problems.
>
> Signed-off-by: Kővágó, Zoltán <dirty.ice...@gmail.com>
> ---
> 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 aa68814..ff61d42 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);
Instead of using a GQueue, I think you could use a GArray, and use
g_strjoin() here.
> +
> + list = g_hash_table_lookup(ov->unprocessed_opts, key);
> + if (list && out_key) {
> + *out_key = g_strdup(key);
or just *out_key = key; key = NULL; (g_free accepts 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 +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);
It would make sense to add an assert(ov->repeated_name == NULL) imho,
this could catch potential leaks.
> + 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_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 +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_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 +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_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 +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_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 +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',
make check fails with this patch, you need to update
tests/qapi-schema/qapi-schema-test.out.
> + 'sub1': 'UserDefOptionsSub' } }
>
> # testing event
> { 'struct': 'EventStructOne',
> 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;
> }
> --
> 2.4.5
>
>
--
Marc-André Lureau
