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 <armbru@redhat.com>
Message-Id: <1488544368-30622-26-git-send-email-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 2de6377cca30cf813544f7045b7e59966ac90e06..150f6bf2c790dabfe0dfef6e7d0633f0f65e1c55 100644 (file)
--- 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 962ba1df35fa9e4bc12484c953bda740dfeedcca..e87709db5c925ceb26a4829fa3f558b72faf3b5d 100644 (file)
--- 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 7c91a50a4c65f2099f0762b5d06f5c8235b8260a..1a1b62012b8e493cda237a021896fa097f4596fa 100644 (file)
--- 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 c50dc4bbbf282c42db418a838a59f7c737b9c588..026d25b76779a5657cf92008b44d413387cc9a2e 100644 (file)
--- 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 e6e93f02e6d145504aaca42df5617e59b1bf5cf9..43a09d147d5ea9521dd8e2dc0bb7622801c3c83c 100644 (file)
--- 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 eafcdf462590a09271ae7a0a92ce22c73cd1d065..34065ba7dd986acae9842bd948a16ca400b5a86d 100644 (file)
--- 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 ?: "<anonymous>");
             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 "<anonymous>";
     }
 
     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 f126cd95a90f2983208237ac2dd64697456df151..806b01ae3a2d34c57ed78343c8e6b7922fed7886 100644 (file)
--- 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 9cbb61b2bd0776603d83c934a486cb6b40e6c9f9..339cacf0ad0cf019145b9edd0c209b6570f26c87 100644 (file)
--- 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 96f2491c16c968c49f6c7bad876455e8a4408d07..330b9f321b0bad936aba2daf520eb415584aede2 100644 (file)
--- 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 d0f764699b1f472d1371583af50104423a87f3b0..b93fd330a8d2475c5c011f82914866ae8e526066 100644 (file)
--- 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 9f3a826353a0c33aa576f436c9aee69e02734950..87d4a77e4abef35e94c3adb2084a089b2b7d776a 100644 (file)
--- 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 70cee65cd080a91c9ddd8deb69a1bee06892c6e4..fbe380acbe8c439cae25eddce198c88abc69d9dd 100644 (file)
--- 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);
 }