From a8aec6de2ac1a5e36989fdfba29067b361009b75 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 3 Mar 2017 13:32:35 +0100 Subject: qapi: Drop string input visitor method optional() visit_optional() is to be called only between visit_start_struct() and visit_end_struct(). Visitors that don't support struct visits, i.e. don't implement start_struct(), end_struct(), have no use for it. Clarify documentation. The string input visitor doesn't support struct visits. Its parse_optional() is therefore useless. Drop it. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <1488544368-30622-16-git-send-email-armbru@redhat.com> --- qapi/string-input-visitor.c | 13 ------------- 1 file changed, 13 deletions(-) (limited to 'qapi/string-input-visitor.c') diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index 8dfa561252..1a855c5474 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -314,18 +314,6 @@ static void parse_type_number(Visitor *v, const char *name, double *obj, *obj = val; } -static void parse_optional(Visitor *v, const char *name, bool *present) -{ - StringInputVisitor *siv = to_siv(v); - - if (!siv->string) { - *present = false; - return; - } - - *present = true; -} - static void string_input_free(Visitor *v) { StringInputVisitor *siv = to_siv(v); @@ -351,7 +339,6 @@ Visitor *string_input_visitor_new(const char *str) v->visitor.start_list = start_list; v->visitor.next_list = next_list; v->visitor.end_list = end_list; - v->visitor.optional = parse_optional; v->visitor.free = string_input_free; v->string = str; -- cgit 1.4.1 From f332e830e38b3ff3953ef02ac04e409ae53769c5 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 3 Mar 2017 13:32:36 +0100 Subject: qapi: Make string input and opts visitor require non-null input The string input visitor tries to cope with null input. Null input isn't used anywhere, and isn't covered by tests. Unsurprisingly, it doesn't fully work: start_list() crashes because it passes the input via parse_str() to strtoll() unchecked. Make string_input_visitor_new() assert its argument isn't null, and drop the code trying to deal with null input. The opts visitor crashes when you try to actually visit something with null input. Make opts_visitor_new() assert its argument isn't null, mostly for clarity. qobject_input_visitor_new() already asserts its argument isn't null. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <1488544368-30622-17-git-send-email-armbru@redhat.com> --- qapi/opts-visitor.c | 1 + qapi/string-input-visitor.c | 54 ++++++++++++++------------------------------- 2 files changed, 18 insertions(+), 37 deletions(-) (limited to 'qapi/string-input-visitor.c') diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index a0a7c0e734..c50dc4bbbf 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -528,6 +528,7 @@ opts_visitor_new(const QemuOpts *opts) { OptsVisitor *ov; + assert(opts); ov = g_malloc0(sizeof *ov); ov->visitor.type = VISITOR_INPUT; diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index 1a855c5474..f126cd95a9 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -182,12 +182,6 @@ static void parse_type_int64(Visitor *v, const char *name, int64_t *obj, { StringInputVisitor *siv = to_siv(v); - if (!siv->string) { - error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", - "integer"); - return; - } - if (parse_str(siv, name, errp) < 0) { return; } @@ -242,13 +236,7 @@ static void parse_type_size(Visitor *v, const char *name, uint64_t *obj, Error *err = NULL; uint64_t val; - if (siv->string) { - parse_option_size(name, siv->string, &val, &err); - } else { - error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", - "size"); - return; - } + parse_option_size(name, siv->string, &val, &err); if (err) { error_propagate(errp, err); return; @@ -262,19 +250,17 @@ static void parse_type_bool(Visitor *v, const char *name, bool *obj, { StringInputVisitor *siv = to_siv(v); - if (siv->string) { - if (!strcasecmp(siv->string, "on") || - !strcasecmp(siv->string, "yes") || - !strcasecmp(siv->string, "true")) { - *obj = true; - return; - } - if (!strcasecmp(siv->string, "off") || - !strcasecmp(siv->string, "no") || - !strcasecmp(siv->string, "false")) { - *obj = false; - return; - } + if (!strcasecmp(siv->string, "on") || + !strcasecmp(siv->string, "yes") || + !strcasecmp(siv->string, "true")) { + *obj = true; + return; + } + if (!strcasecmp(siv->string, "off") || + !strcasecmp(siv->string, "no") || + !strcasecmp(siv->string, "false")) { + *obj = false; + return; } error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", @@ -285,13 +271,8 @@ static void parse_type_str(Visitor *v, const char *name, char **obj, Error **errp) { StringInputVisitor *siv = to_siv(v); - if (siv->string) { - *obj = g_strdup(siv->string); - } else { - *obj = NULL; - error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", - "string"); - } + + *obj = g_strdup(siv->string); } static void parse_type_number(Visitor *v, const char *name, double *obj, @@ -302,10 +283,8 @@ static void parse_type_number(Visitor *v, const char *name, double *obj, double val; errno = 0; - if (siv->string) { - val = strtod(siv->string, &endp); - } - if (!siv->string || errno || endp == siv->string || *endp) { + val = strtod(siv->string, &endp); + if (errno || endp == siv->string || *endp) { error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "number"); return; @@ -327,6 +306,7 @@ Visitor *string_input_visitor_new(const char *str) { StringInputVisitor *v; + assert(str); v = g_malloc0(sizeof(*v)); v->visitor.type = VISITOR_INPUT; -- cgit 1.4.1 From a4a1c70dc759e5b81627e96564f344ab43ea86eb Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 3 Mar 2017 13:32:45 +0100 Subject: qapi: Make input visitors detect unvisited list tails Fix the design flaw demonstrated in the previous commit: new method check_list() lets input visitors report that unvisited input remains for a list, exactly like check_struct() lets them report that unvisited input remains for a struct or union. Implement the method for the qobject input visitor (straightforward), and the string input visitor (less so, due to the magic list syntax there). The opts visitor's list magic is even more impenetrable, and all I can do there today is a stub with a FIXME comment. No worse than before. Signed-off-by: Markus Armbruster Message-Id: <1488544368-30622-26-git-send-email-armbru@redhat.com> Reviewed-by: Eric Blake --- hw/ppc/spapr_drc.c | 5 +++++ include/qapi/visitor-impl.h | 3 +++ include/qapi/visitor.h | 13 +++++++++++++ qapi/opts-visitor.c | 11 +++++++++++ qapi/qapi-visit-core.c | 8 ++++++++ qapi/qobject-input-visitor.c | 37 +++++++++++++++++++++++++++++++------ qapi/string-input-visitor.c | 30 ++++++++++++++++++++++++++++++ qapi/trace-events | 1 + scripts/qapi-visit.py | 3 +++ tests/test-opts-visitor.c | 2 +- tests/test-qobject-input-visitor.c | 9 +++++++-- tests/test-string-input-visitor.c | 4 +++- 12 files changed, 116 insertions(+), 10 deletions(-) (limited to 'qapi/string-input-visitor.c') diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index 2de6377cca..150f6bf2c7 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -326,7 +326,12 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name, return; } } + visit_check_list(v, &err); visit_end_list(v, NULL); + if (err) { + error_propagate(errp, err); + return; + } break; } default: diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index 962ba1df35..e87709db5c 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -61,6 +61,9 @@ struct Visitor /* Must be set */ GenericList *(*next_list)(Visitor *v, GenericList *tail, size_t size); + /* Optional; intended for input visitors */ + void (*check_list)(Visitor *v, Error **errp); + /* Must be set */ void (*end_list)(Visitor *v, void **list); diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index 7c91a50a4c..1a1b62012b 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -369,6 +369,19 @@ void visit_start_list(Visitor *v, const char *name, GenericList **list, */ GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size); +/* + * Prepare for completing a list visit. + * + * @errp obeys typical error usage, and reports failures such as + * unvisited list tail remaining in the input stream. + * + * Should be called prior to visit_end_list() if all other + * intermediate visit steps were successful, to allow the visitor one + * last chance to report errors. May be skipped on a cleanup path, + * where there is no need to check for further errors. + */ +void visit_check_list(Visitor *v, Error **errp); + /* * Complete a list visit started earlier. * diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index c50dc4bbbf..026d25b767 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -272,6 +272,16 @@ opts_next_list(Visitor *v, GenericList *tail, size_t size) } +static void +opts_check_list(Visitor *v, Error **errp) +{ + /* + * FIXME should set error when unvisited elements remain. Mostly + * harmless, as the generated visits always visit all elements. + */ +} + + static void opts_end_list(Visitor *v, void **obj) { @@ -539,6 +549,7 @@ opts_visitor_new(const QemuOpts *opts) ov->visitor.start_list = &opts_start_list; ov->visitor.next_list = &opts_next_list; + ov->visitor.check_list = &opts_check_list; ov->visitor.end_list = &opts_end_list; ov->visitor.type_int64 = &opts_type_int64; diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index e6e93f02e6..43a09d147d 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -90,6 +90,14 @@ GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size) return v->next_list(v, tail, size); } +void visit_check_list(Visitor *v, Error **errp) +{ + trace_visit_check_list(v); + if (v->check_list) { + v->check_list(v, errp); + } +} + void visit_end_list(Visitor *v, void **obj) { trace_visit_end_list(v, obj); diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c index eafcdf4625..34065ba7dd 100644 --- a/qapi/qobject-input-visitor.c +++ b/qapi/qobject-input-visitor.c @@ -51,7 +51,8 @@ static QObjectInputVisitor *to_qiv(Visitor *v) return container_of(v, QObjectInputVisitor, visitor); } -static const char *full_name(QObjectInputVisitor *qiv, const char *name) +static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name, + int n) { StackObject *so; char buf[32]; @@ -63,8 +64,10 @@ static const char *full_name(QObjectInputVisitor *qiv, const char *name) } QSLIST_FOREACH(so , &qiv->stack, node) { - if (qobject_type(so->obj) == QTYPE_QDICT) { - g_string_prepend(qiv->errname, name); + if (n) { + n--; + } else if (qobject_type(so->obj) == QTYPE_QDICT) { + g_string_prepend(qiv->errname, name ?: ""); g_string_prepend_c(qiv->errname, '.'); } else { snprintf(buf, sizeof(buf), "[%u]", so->index); @@ -72,18 +75,24 @@ static const char *full_name(QObjectInputVisitor *qiv, const char *name) } name = so->name; } + assert(!n); if (name) { g_string_prepend(qiv->errname, name); } else if (qiv->errname->str[0] == '.') { g_string_erase(qiv->errname, 0, 1); - } else { + } else if (!qiv->errname->str[0]) { return ""; } return qiv->errname->str; } +static const char *full_name(QObjectInputVisitor *qiv, const char *name) +{ + return full_name_nth(qiv, name, 0); +} + static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv, const char *name, bool consume) @@ -260,15 +269,30 @@ static GenericList *qobject_input_next_list(Visitor *v, GenericList *tail, size_t size) { QObjectInputVisitor *qiv = to_qiv(v); - StackObject *so = QSLIST_FIRST(&qiv->stack); + StackObject *tos = QSLIST_FIRST(&qiv->stack); + + assert(tos && tos->obj && qobject_type(tos->obj) == QTYPE_QLIST); - if (!so->entry) { + if (!tos->entry) { return NULL; } tail->next = g_malloc0(size); return tail->next; } +static void qobject_input_check_list(Visitor *v, Error **errp) +{ + QObjectInputVisitor *qiv = to_qiv(v); + StackObject *tos = QSLIST_FIRST(&qiv->stack); + + assert(tos && tos->obj && qobject_type(tos->obj) == QTYPE_QLIST); + + if (tos->entry) { + error_setg(errp, "Only %u list elements expected in %s", + tos->index + 1, full_name_nth(qiv, NULL, 1)); + } +} + static void qobject_input_start_alternate(Visitor *v, const char *name, GenericAlternate **obj, size_t size, @@ -471,6 +495,7 @@ Visitor *qobject_input_visitor_new(QObject *obj) v->visitor.end_struct = qobject_input_pop; v->visitor.start_list = qobject_input_start_list; v->visitor.next_list = qobject_input_next_list; + v->visitor.check_list = qobject_input_check_list; v->visitor.end_list = qobject_input_pop; v->visitor.start_alternate = qobject_input_start_alternate; v->visitor.type_int64 = qobject_input_type_int64; diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index f126cd95a9..806b01ae3a 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -170,6 +170,35 @@ static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) return tail->next; } +static void check_list(Visitor *v, Error **errp) +{ + const StringInputVisitor *siv = to_siv(v); + Range *r; + GList *cur_range; + + if (!siv->ranges || !siv->cur_range) { + return; + } + + r = siv->cur_range->data; + if (!r) { + return; + } + + if (!range_contains(r, siv->cur)) { + cur_range = g_list_next(siv->cur_range); + if (!cur_range) { + return; + } + r = cur_range->data; + if (!r) { + return; + } + } + + error_setg(errp, "Range contains too many values"); +} + static void end_list(Visitor *v, void **obj) { StringInputVisitor *siv = to_siv(v); @@ -318,6 +347,7 @@ Visitor *string_input_visitor_new(const char *str) v->visitor.type_number = parse_type_number; v->visitor.start_list = start_list; v->visitor.next_list = next_list; + v->visitor.check_list = check_list; v->visitor.end_list = end_list; v->visitor.free = string_input_free; diff --git a/qapi/trace-events b/qapi/trace-events index 9cbb61b2bd..339cacf0ad 100644 --- a/qapi/trace-events +++ b/qapi/trace-events @@ -8,6 +8,7 @@ visit_end_struct(void *v, void *obj) "v=%p obj=%p" visit_start_list(void *v, const char *name, void *obj, size_t size) "v=%p name=%s obj=%p size=%zu" visit_next_list(void *v, void *tail, size_t size) "v=%p tail=%p size=%zu" +visit_check_list(void *v) "v=%p" visit_end_list(void *v, void *obj) "v=%p obj=%p" visit_start_alternate(void *v, const char *name, void *obj, size_t size, bool promote_int) "v=%p name=%s obj=%p size=%zu promote_int=%d" diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 96f2491c16..330b9f321b 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -133,6 +133,9 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error } } + if (!err) { + visit_check_list(v, &err); + } visit_end_list(v, (void **)obj); if (err && visit_is_input(v)) { qapi_free_%(c_name)s(*obj); diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c index d0f764699b..b93fd330a8 100644 --- a/tests/test-opts-visitor.c +++ b/tests/test-opts-visitor.c @@ -199,8 +199,8 @@ test_opts_range_unvisited(void) g_assert_cmpint(tail->value, ==, 1); tail = (intList *)visit_next_list(v, (GenericList *)tail, sizeof(*list)); g_assert(tail); + visit_check_list(v, &error_abort); /* BUG: unvisited tail not reported */ visit_end_list(v, (void **)&list); - /* BUG: unvisited tail not reported; actually not reportable by design */ visit_check_struct(v, &error_abort); visit_end_struct(v, NULL); diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c index 9f3a826353..87d4a77e4a 100644 --- a/tests/test-qobject-input-visitor.c +++ b/tests/test-qobject-input-visitor.c @@ -933,6 +933,7 @@ static void test_visitor_in_fail_list(TestInputVisitorData *data, const void *unused) { int64_t i64 = -1; + Error *err = NULL; Visitor *v; /* Unvisited list tail */ @@ -944,14 +945,16 @@ static void test_visitor_in_fail_list(TestInputVisitorData *data, g_assert_cmpint(i64, ==, 1); visit_type_int(v, NULL, &i64, &error_abort); g_assert_cmpint(i64, ==, 2); + visit_check_list(v, &err); + error_free_or_abort(&err); visit_end_list(v, NULL); - /* BUG: unvisited tail not reported; actually not reportable by design */ } static void test_visitor_in_fail_list_nested(TestInputVisitorData *data, const void *unused) { int64_t i64 = -1; + Error *err = NULL; Visitor *v; /* Unvisited nested list tail */ @@ -964,8 +967,10 @@ static void test_visitor_in_fail_list_nested(TestInputVisitorData *data, visit_start_list(v, NULL, NULL, 0, &error_abort); visit_type_int(v, NULL, &i64, &error_abort); g_assert_cmpint(i64, ==, 1); + visit_check_list(v, &err); + error_free_or_abort(&err); visit_end_list(v, NULL); - /* BUG: unvisited tail not reported; actually not reportable by design */ + visit_check_list(v, &error_abort); visit_end_list(v, NULL); } diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c index 70cee65cd0..fbe380acbe 100644 --- a/tests/test-string-input-visitor.c +++ b/tests/test-string-input-visitor.c @@ -169,8 +169,10 @@ static void test_visitor_in_intList(TestInputVisitorData *data, g_assert_cmpint(tail->value, ==, 2); tail = (int64List *)visit_next_list(v, (GenericList *)tail, sizeof(*res)); g_assert(tail); + + visit_check_list(v, &err); + error_free_or_abort(&err); visit_end_list(v, (void **)&res); - /* BUG: unvisited tail not reported; actually not reportable by design */ qapi_free_int64List(res); } -- cgit 1.4.1