[Qemu-devel] [PATCH v10 12/13] qapi: Delete unused visit_start_union()

[Eric Blake]

Commit cee2dedb noticed that if you have a partial flat union
(such as if an input parse failed due to a missing
discriminator), calling the dealloc visitor could result in
trying to dereference a NULL pointer if we attempted to visit
an object branch without an earlier successful call to
visit_start_implicit_struct() allocating the pointer for that
branch. But the "fix" it implemented requires the use of a
'.data' member in the union, which may or may not be the same
size as other branches of the union (consider a 32-bit platform
where one of the branches is an int64), which feels fairly dirty.
Plus, as mentioned in that commit, it only works if you can
assume that '.data' would be zero-initialized even if '.kind' was
uninitialized, which is rather poor logic: our usage of
visit_start_struct() happens to zero-initialize both fields,
which means '.kind' is never truly uninitialized - but if we
changed visit_start_struct() to use g_new() instead of g_new0(),
then '.data' would not be any more reliable as a condition on
whether to visit the branch matching '.kind', regardless of
whether '.kind' was 0).

Menawhile, now that we have just inlined the fields of all flat
unions, there is no longer the possibility of a null pointer to
dereference in the first place.  Where the branch structure used
to be separately allocated by visit_start_implicit_struct(), it
is now just pointing to a subset of the memory already
zero-allocated by visit_start_struct().

Thus, we can instead fix things to delete the misguided
visit_start_union(), as it is no longer providing any benefit.
And it finishes the cleanup we started in commit 7c91aabd when
we deleted visit_end_union().  Generated code changes as follows:

|@@ -2366,9 +2363,6 @@ void visit_type_ChardevBackend(Visitor *
|     if (err) {
|         goto out_obj;
|     }
|-    if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
|-        goto out_obj;
|-    }
|     switch ((*obj)->type) {
|     case CHARDEV_BACKEND_KIND_FILE:
|         visit_type_ChardevFile(v, "data", &(*obj)->u.file, &err);

Signed-off-by: Eric Blake <address@hidden>

---
v10: retitle, hoist earlier in series, rebase, drop R-b
v9: no change
v8: rebase to 'name' motion
v7: rebase to earlier context changes, simplify 'obj && !*obj'
condition based on contract
v6: rebase due to deferring 7/46, and gen_err_check() improvements;
rewrite gen_visit_implicit_struct() more like other patterns
---
 include/qapi/visitor.h      |  1 -
 include/qapi/visitor-impl.h |  2 --
 scripts/qapi-visit.py       |  3 ---
 qapi/qapi-visit-core.c      |  8 --------
 qapi/qapi-dealloc-visitor.c | 26 --------------------------
 5 files changed, 40 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index c131a32..b8ae1b5 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -80,6 +80,5 @@ void visit_type_str(Visitor *v, const char *name, char **obj, 
Error **errp);
 void visit_type_number(Visitor *v, const char *name, double *obj,
                        Error **errp);
 void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp);
-bool visit_start_union(Visitor *v, bool data_present, Error **errp);

 #endif
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 7905a28..c4af3e0 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -58,8 +58,6 @@ struct Visitor

     /* May be NULL; most useful for input visitors. */
     void (*optional)(Visitor *v, const char *name, bool *present);
-
-    bool (*start_union)(Visitor *v, bool data_present, Error **errp);
 };

 void input_type_enum(Visitor *v, const char *name, int *obj,
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 68354d8..02f0122 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -246,9 +246,6 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
%(c_name)s **obj, Error
     if variants:
         ret += gen_err_check(label='out_obj')
         ret += mcgen('''
-    if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
-        goto out_obj;
-    }
     switch ((*obj)->%(c_name)s) {
 ''',
                      c_name=c_name(variants.tag_member.name))
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 6fa66f1..976106e 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -60,14 +60,6 @@ void visit_end_list(Visitor *v)
     v->end_list(v);
 }

-bool visit_start_union(Visitor *v, bool data_present, Error **errp)
-{
-    if (v->start_union) {
-        return v->start_union(v, data_present, errp);
-    }
-    return true;
-}
-
 bool visit_optional(Visitor *v, const char *name, bool *present)
 {
     if (v->optional) {
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 6667e8c..4eae555 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -169,31 +169,6 @@ static void qapi_dealloc_type_enum(Visitor *v, const char 
*name, int *obj,
 {
 }

-/* If there's no data present, the dealloc visitor has nothing to free.
- * Thus, indicate to visitor code that the subsequent union fields can
- * be skipped. This is not an error condition, since the cleanup of the
- * rest of an object can continue unhindered, so leave errp unset in
- * these cases.
- *
- * NOTE: In cases where we're attempting to deallocate an object that
- * may have missing fields, the field indicating the union type may
- * be missing. In such a case, it's possible we don't have enough
- * information to differentiate data_present == false from a case where
- * data *is* present but happens to be a scalar with a value of 0.
- * This is okay, since in the case of the dealloc visitor there's no
- * work that needs to done in either situation.
- *
- * The current inability in QAPI code to more thoroughly verify a union
- * type in such cases will likely need to be addressed if we wish to
- * implement this interface for other types of visitors in the future,
- * however.
- */
-static bool qapi_dealloc_start_union(Visitor *v, bool data_present,
-                                     Error **errp)
-{
-    return data_present;
-}
-
 Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v)
 {
     return &v->visitor;
@@ -224,7 +199,6 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
     v->visitor.type_str = qapi_dealloc_type_str;
     v->visitor.type_number = qapi_dealloc_type_number;
     v->visitor.type_any = qapi_dealloc_type_anything;
-    v->visitor.start_union = qapi_dealloc_start_union;

     QTAILQ_INIT(&v->stack);

-- 
2.5.0