[Qemu-devel] [PATCH v6 00/23] qapi visitor cleanups (post-introspection cleanups subset E)

[Qemu-devel] [PATCH v6 00/23] qapi visitor cleanups (post-introspection cleanups subset E)

[Eric Blake]

Pending prerequisites:
+ Markus' "typedefs: Put them back into alphabetical order"
https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04417.html
+ Markus' qapi-next branch
http://repo.or.cz/qemu/armbru.git/shortlog/refs/heads/qapi-next
+ My v13 subset D patches:
https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04732.html

Also available as a tag at this location:
git fetch git://repo.or.cz/qemu/ericb.git qapi-cleanupv6e

and will soon be part of my branch with the rest of the v5 series, at:
http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi

v6 notes:
My set of patches related to qapi visitors has grown, and it's time
that I post it on list again.  Of course, since this is all 2.6
material, and there's already lots of patches earlier in the queue,
I may need a v7 to pick up rebase changes.

A lot of the new patches in this series are based on fallout from
implementing an early RFC posted against a v5 review:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg06878.html

Backport diff from v5:

001/23:[down] 'qapi: Make all visitors supply int64/uint64 callbacks'
002/23:[down] 'qapi: Require int64/uint64 implementation'
003/23:[down] 'qapi: Consolidate visitor integer callbacks'
004/23:[down] 'qapi: Don't cast Enum* to int*'
005/23:[----] [--] 'qmp: Fix reference-counting of qnull on empty output visit'
006/23:[----] [--] 'qapi: Don't abuse stack to track qmp-output root'
007/23:[0100] [FC] 'qapi: Document visitor interfaces'
008/23:[down] 'qapi: Drop unused error argument for list and implicit struct'
009/23:[down] 'hmp: Improve use of qapi visitor'
010/23:[down] 'vl: Improve use of qapi visitor'
011/23:[down] 'ppc: Improve use of qapi visitors'
012/23:[down] 'balloon: Improve use of qapi visitor'
013/23:[down] 'qapi: Add type.is_empty() helper'
014/23:[down] 'qapi: Fix command with named empty argument type'
015/23:[down] 'qapi: Improve generated event use of qapi visitor'
016/23:[down] 'qapi: Track all failures between visit_start/stop'
017/23:[down] 'qapi: Eliminate empty visit_type_FOO_fields'
018/23:[down] 'qapi: Canonicalize missing object to :empty'
019/23:[down] 'qapi-visit: Unify struct and union visit'
020/23:[0029] [FC] 'qapi: Rework deallocation of partial struct'
021/23:[down] 'qapi: Simplify extra member error reporting in input visitors'
022/23:[down] 'qapi: Split visit_end_struct() into pieces'
023/23:[0174] [FC] 'qapi: Change visit_type_FOO() to no longer return partial objects'

Subset F (and more?) will come later.

In v5:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05410.html
I _did_ rearrange patches to try and group related features:

1-2: Groundwork cleanups
3-5: Add more test cases
6-16: Front-end cleanups
17-18: Introspection output cleanups
19-20: 'alternate' type cleanups
21-29: qapi visitor cleanups
30-45: qapi-ify netdev_add
46: add qapi shorthand for flat unions

Lots of fixes based on additional testing, and rebased to
track other changes that happened in the meantime.  The series
is huge; I can split off smaller portions as requested.

In v4:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02580.html
add some more clean up patches
rebase to Markus' recent work
pull in part of Zoltán's work to make netdev_add a flat union,
further enhancing it to be introspectible

I might be able to rearrange some of these patches, or separate
it into smaller independent series, if requested; but I'm
posting now to get review started.

In v3:
https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02059.html
redo cleanup of dealloc of partial struct
add patches to make all visit_type_*() avoid leaks on failure
add patches to allow boxed command arguments and events

In v2:
https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg00900.html
rebase to Markus' v3 series
rework how comments are emitted for fields inherited from base
additional patches added for deleting colliding 'void *data'
documentation updates to match code changes

v1 was here:
https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg05266.html
https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg05325.html

Eric Blake (23):
  qapi: Make all visitors supply int64/uint64 callbacks
  qapi: Require int64/uint64 implementation
  qapi: Consolidate visitor integer callbacks
  qapi: Don't cast Enum* to int*
  qmp: Fix reference-counting of qnull on empty output visit
  qapi: Don't abuse stack to track qmp-output root
  qapi: Document visitor interfaces
  qapi: Drop unused error argument for list and implicit struct
  hmp: Improve use of qapi visitor
  vl: Improve use of qapi visitor
  ppc: Improve use of qapi visitors
  balloon: Improve use of qapi visitor
  qapi: Add type.is_empty() helper
  qapi: Fix command with named empty argument type
  qapi: Improve generated event use of qapi visitor
  qapi: Track all failures between visit_start/stop
  qapi: Eliminate empty visit_type_FOO_fields
  qapi: Canonicalize missing object to :empty
  qapi-visit: Unify struct and union visit
  qapi: Rework deallocation of partial struct
  qapi: Simplify extra member error reporting in input visitors
  qapi: Split visit_end_struct() into pieces
  qapi: Change visit_type_FOO() to no longer return partial objects

 hmp.c                                   |  22 ++-
 hw/ppc/spapr_drc.c                      |  16 ++-
 hw/virtio/virtio-balloon.c              |  21 +--
 include/qapi/visitor-impl.h             |  71 +++++++---
 include/qapi/visitor.h                  | 232 +++++++++++++++++++++++++++++---
 qapi/opts-visitor.c                     |  43 +++---
 qapi/qapi-dealloc-visitor.c             |  59 +++-----
 qapi/qapi-visit-core.c                  | 196 +++++++++++----------------
 qapi/qmp-input-visitor.c                |  92 +++++++------
 qapi/qmp-output-visitor.c               |  94 +++++++------
 qapi/string-input-visitor.c             |  26 +++-
 qapi/string-output-visitor.c            |  20 ++-
 qom/object.c                            |   5 +-
 scripts/qapi-commands.py                |   7 +-
 scripts/qapi-event.py                   |  29 ++--
 scripts/qapi-types.py                   |   6 +-
 scripts/qapi-visit.py                   | 225 +++++++++++++++----------------
 scripts/qapi.py                         |  30 +++--
 tests/qapi-schema/event-case.out        |   2 +-
 tests/qapi-schema/flat-union-empty.out  |   1 +
 tests/qapi-schema/ident-with-escape.out |   1 +
 tests/qapi-schema/indented-expr.out     |   4 +-
 tests/qapi-schema/qapi-schema-test.json |   2 +
 tests/qapi-schema/qapi-schema-test.out  |  47 ++++++-
 tests/qapi-schema/union-clash-data.out  |   2 +
 tests/qapi-schema/union-empty.out       |   1 +
 tests/test-qmp-commands.c               |  18 ++-
 tests/test-qmp-input-strict.c           |  19 ++-
 tests/test-qmp-input-visitor.c          |  10 +-
 tests/test-qmp-output-visitor.c         |   2 +
 vl.c                                    |  29 ++--
 31 files changed, 797 insertions(+), 535 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v6 01/23] qapi: Make all visitors supply int64/uint64 callbacks

[Eric Blake]

Our qapi visitor contract supports multiple integer visitors:
type_int (64-bit signed; mandatory), type_int64 (64-bit signed,
optional), type_uint64 (64-bit unsigned, optional), and
type_size (64-bit unsigned, optional, with the possibility of
parsing differently than type_uint64 for the case of suffixed
sizes).  But the default code treats any implementation without
type_uint64 as just falling back on type_int, which can cause
confusing results for values larger than LLONG_MAX (such as
having to pass in a negative 2s complement value on input,
and getting a negative result on output).

This patch does not fully address the disparity in parsing,
but does move towards a cleaner situation where EVERY visitor
provides both signed and unsigned variants as entry points;
then each client can either implement sane differences between
the two, or document in place with a FIXME that there is
munging going on for large values.  The next patch will then
clean up the code to no longer allow conditional type_uint64.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: new patch, but stems from v5 23/46
---
 qapi/opts-visitor.c          |  5 +++--
 qapi/qapi-dealloc-visitor.c  | 19 ++++++++++---------
 qapi/qmp-input-visitor.c     | 23 ++++++++++++++++++++---
 qapi/qmp-output-visitor.c    | 15 ++++++++++++---
 qapi/string-input-visitor.c  | 22 +++++++++++++++++++---
 qapi/string-output-visitor.c | 16 +++++++++++++---
 6 files changed, 77 insertions(+), 23 deletions(-)

diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index ef5fb8b..bc2b976 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -354,7 +354,7 @@ opts_type_bool(Visitor *v, bool *obj, const char *name, Error **errp)


 static void
-opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
+opts_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp)
 {
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
     const QemuOpt *opt;
@@ -522,7 +522,8 @@ opts_visitor_new(const QemuOpts *opts)
      */
     ov->visitor.type_enum = &input_type_enum;

-    ov->visitor.type_int    = &opts_type_int;
+    ov->visitor.type_int    = &opts_type_int64;
+    ov->visitor.type_int64  = &opts_type_int64;
     ov->visitor.type_uint64 = &opts_type_uint64;
     ov->visitor.type_size   = &opts_type_size;
     ov->visitor.type_bool   = &opts_type_bool;
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 737deab..5d1b2e6 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -136,8 +136,13 @@ static void qapi_dealloc_type_str(Visitor *v, char **obj, const char *name,
     }
 }

-static void qapi_dealloc_type_int(Visitor *v, int64_t *obj, const char *name,
-                                  Error **errp)
+static void qapi_dealloc_type_int64(Visitor *v, int64_t *obj,
+                                    const char *name, Error **errp)
+{
+}
+
+static void qapi_dealloc_type_uint64(Visitor *v, uint64_t *obj,
+                                     const char *name, Error **errp)
 {
 }

@@ -159,11 +164,6 @@ static void qapi_dealloc_type_anything(Visitor *v, QObject **obj,
     }
 }

-static void qapi_dealloc_type_size(Visitor *v, uint64_t *obj, const char *name,
-                                   Error **errp)
-{
-}
-
 static void qapi_dealloc_type_enum(Visitor *v, int *obj,
                                    const char * const strings[],
                                    const char *kind, const char *name,
@@ -220,12 +220,13 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
     v->visitor.next_list = qapi_dealloc_next_list;
     v->visitor.end_list = qapi_dealloc_end_list;
     v->visitor.type_enum = qapi_dealloc_type_enum;
-    v->visitor.type_int = qapi_dealloc_type_int;
+    v->visitor.type_int = qapi_dealloc_type_int64;
+    v->visitor.type_int64 = qapi_dealloc_type_int64;
+    v->visitor.type_uint64 = qapi_dealloc_type_uint64;
     v->visitor.type_bool = qapi_dealloc_type_bool;
     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.type_size = qapi_dealloc_type_size;
     v->visitor.start_union = qapi_dealloc_start_union;

     QTAILQ_INIT(&v->stack);
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 932b5f3..96dafcb 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -224,8 +224,23 @@ static void qmp_input_get_next_type(Visitor *v, QType *type, bool promote_int,
     }
 }

-static void qmp_input_type_int(Visitor *v, int64_t *obj, const char *name,
-                               Error **errp)
+static void qmp_input_type_int64(Visitor *v, int64_t *obj, const char *name,
+                                 Error **errp)
+{
+    QmpInputVisitor *qiv = to_qiv(v);
+    QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true));
+
+    if (!qint) {
+        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
+                   "integer");
+        return;
+    }
+
+    *obj = qint_get_int(qint);
+}
+
+static void qmp_input_type_uint64(Visitor *v, uint64_t *obj, const char *name,
+                                  Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
     QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true));
@@ -341,7 +356,9 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
     v->visitor.next_list = qmp_input_next_list;
     v->visitor.end_list = qmp_input_end_list;
     v->visitor.type_enum = input_type_enum;
-    v->visitor.type_int = qmp_input_type_int;
+    v->visitor.type_int = qmp_input_type_int64;
+    v->visitor.type_int64 = qmp_input_type_int64;
+    v->visitor.type_uint64 = qmp_input_type_uint64;
     v->visitor.type_bool = qmp_input_type_bool;
     v->visitor.type_str = qmp_input_type_str;
     v->visitor.type_number = qmp_input_type_number;
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index 29899ac..a52f26f 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -158,8 +158,15 @@ static void qmp_output_end_list(Visitor *v, Error **errp)
     qmp_output_pop(qov);
 }

-static void qmp_output_type_int(Visitor *v, int64_t *obj, const char *name,
-                                Error **errp)
+static void qmp_output_type_int64(Visitor *v, int64_t *obj, const char *name,
+                                  Error **errp)
+{
+    QmpOutputVisitor *qov = to_qov(v);
+    qmp_output_add(qov, name, qint_from_int(*obj));
+}
+
+static void qmp_output_type_uint64(Visitor *v, uint64_t *obj, const char *name,
+                                   Error **errp)
 {
     QmpOutputVisitor *qov = to_qov(v);
     qmp_output_add(qov, name, qint_from_int(*obj));
@@ -241,7 +248,9 @@ QmpOutputVisitor *qmp_output_visitor_new(void)
     v->visitor.next_list = qmp_output_next_list;
     v->visitor.end_list = qmp_output_end_list;
     v->visitor.type_enum = output_type_enum;
-    v->visitor.type_int = qmp_output_type_int;
+    v->visitor.type_int = qmp_output_type_int64;
+    v->visitor.type_int64 = qmp_output_type_int64;
+    v->visitor.type_uint64 = qmp_output_type_uint64;
     v->visitor.type_bool = qmp_output_type_bool;
     v->visitor.type_str = qmp_output_type_str;
     v->visitor.type_number = qmp_output_type_number;
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index dee780a..0931321 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -179,8 +179,8 @@ end_list(Visitor *v, Error **errp)
     siv->head = true;
 }

-static void parse_type_int(Visitor *v, int64_t *obj, const char *name,
-                           Error **errp)
+static void parse_type_int64(Visitor *v, int64_t *obj, const char *name,
+                             Error **errp)
 {
     StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);

@@ -221,6 +221,20 @@ error:
                "an int64 value or range");
 }

+static void parse_type_uint64(Visitor *v, uint64_t *obj, const char *name,
+                              Error **errp)
+{
+    /* FIXME - don't handle numbers > INT64_MAX as negative */
+    int64_t i;
+    Error *err = NULL;
+    parse_type_int64(v, &i, name, &err);
+    if (err) {
+        error_propagate(errp, err);
+    } else {
+        *obj = i;
+    }
+}
+
 static void parse_type_size(Visitor *v, uint64_t *obj, const char *name,
                             Error **errp)
 {
@@ -330,7 +344,9 @@ StringInputVisitor *string_input_visitor_new(const char *str)
     v = g_malloc0(sizeof(*v));

     v->visitor.type_enum = input_type_enum;
-    v->visitor.type_int = parse_type_int;
+    v->visitor.type_int = parse_type_int64;
+    v->visitor.type_int64 = parse_type_int64;
+    v->visitor.type_uint64 = parse_type_uint64;
     v->visitor.type_size = parse_type_size;
     v->visitor.type_bool = parse_type_bool;
     v->visitor.type_str = parse_type_str;
diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index b86ce2c..83ca6cc 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -116,8 +116,8 @@ static void format_string(StringOutputVisitor *sov, Range *r, bool next,
     }
 }

-static void print_type_int(Visitor *v, int64_t *obj, const char *name,
-                           Error **errp)
+static void print_type_int64(Visitor *v, int64_t *obj, const char *name,
+                             Error **errp)
 {
     StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
     GList *l;
@@ -192,6 +192,14 @@ static void print_type_int(Visitor *v, int64_t *obj, const char *name,
     }
 }

+static void print_type_uint64(Visitor *v, uint64_t *obj, const char *name,
+                             Error **errp)
+{
+    /* FIXME - don't handle numbers > INT64_MAX as negative */
+    int64_t i = *obj;
+    print_type_int64(v, &i, name, errp);
+}
+
 static void print_type_size(Visitor *v, uint64_t *obj, const char *name,
                            Error **errp)
 {
@@ -340,7 +348,9 @@ StringOutputVisitor *string_output_visitor_new(bool human)
     v->string = g_string_new(NULL);
     v->human = human;
     v->visitor.type_enum = output_type_enum;
-    v->visitor.type_int = print_type_int;
+    v->visitor.type_int = print_type_int64;
+    v->visitor.type_int64 = print_type_int64;
+    v->visitor.type_uint64 = print_type_uint64;
     v->visitor.type_size = print_type_size;
     v->visitor.type_bool = print_type_bool;
     v->visitor.type_str = print_type_str;
-- 
2.4.3

[Qemu-devel] [PATCH v6 02/23] qapi: Require int64/uint64 implementation

[Eric Blake]

Now that all visitors supply both type_int64() and type_uint64()
callbacks, we can drop the redundant type_int() callback (the
public interface visit_type_int() remains, but calls into
type_int64() under the hood).

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: new patch, but stems from v5 23/46
---
 include/qapi/visitor-impl.h  |  1 -
 qapi/opts-visitor.c          |  1 -
 qapi/qapi-dealloc-visitor.c  |  1 -
 qapi/qapi-visit-core.c       | 50 ++++++++++++++------------------------------
 qapi/qmp-input-visitor.c     |  1 -
 qapi/qmp-output-visitor.c    |  1 -
 qapi/string-input-visitor.c  |  1 -
 qapi/string-output-visitor.c |  1 -
 8 files changed, 16 insertions(+), 41 deletions(-)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 44a21b7..70326e0 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -36,7 +36,6 @@ struct Visitor
     void (*get_next_type)(Visitor *v, QType *type, bool promote_int,
                           const char *name, Error **errp);

-    void (*type_int)(Visitor *v, int64_t *obj, const char *name, Error **errp);
     void (*type_bool)(Visitor *v, bool *obj, const char *name, Error **errp);
     void (*type_str)(Visitor *v, char **obj, const char *name, Error **errp);
     void (*type_number)(Visitor *v, double *obj, const char *name,
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index bc2b976..8104e42 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -522,7 +522,6 @@ opts_visitor_new(const QemuOpts *opts)
      */
     ov->visitor.type_enum = &input_type_enum;

-    ov->visitor.type_int    = &opts_type_int64;
     ov->visitor.type_int64  = &opts_type_int64;
     ov->visitor.type_uint64 = &opts_type_uint64;
     ov->visitor.type_size   = &opts_type_size;
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 5d1b2e6..5133d37 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -220,7 +220,6 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
     v->visitor.next_list = qapi_dealloc_next_list;
     v->visitor.end_list = qapi_dealloc_end_list;
     v->visitor.type_enum = qapi_dealloc_type_enum;
-    v->visitor.type_int = qapi_dealloc_type_int64;
     v->visitor.type_int64 = qapi_dealloc_type_int64;
     v->visitor.type_uint64 = qapi_dealloc_type_uint64;
     v->visitor.type_bool = qapi_dealloc_type_bool;
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 6d63e40..88bed9c 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -97,19 +97,19 @@ void visit_type_enum(Visitor *v, int *obj, const char * const strings[],

 void visit_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
 {
-    v->type_int(v, obj, name, errp);
+    v->type_int64(v, obj, name, errp);
 }

 void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp)
 {
-    int64_t value;
+    uint64_t value;

     if (v->type_uint8) {
         v->type_uint8(v, obj, name, errp);
     } else {
         value = *obj;
-        v->type_int(v, &value, name, errp);
-        if (value < 0 || value > UINT8_MAX) {
+        v->type_uint64(v, &value, name, errp);
+        if (value > UINT8_MAX) {
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                        name ? name : "null", "uint8_t");
             return;
@@ -120,14 +120,14 @@ void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp)

 void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Error **errp)
 {
-    int64_t value;
+    uint64_t value;

     if (v->type_uint16) {
         v->type_uint16(v, obj, name, errp);
     } else {
         value = *obj;
-        v->type_int(v, &value, name, errp);
-        if (value < 0 || value > UINT16_MAX) {
+        v->type_uint64(v, &value, name, errp);
+        if (value > UINT16_MAX) {
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                        name ? name : "null", "uint16_t");
             return;
@@ -138,14 +138,14 @@ void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Error **errp

 void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, Error **errp)
 {
-    int64_t value;
+    uint64_t value;

     if (v->type_uint32) {
         v->type_uint32(v, obj, name, errp);
     } else {
         value = *obj;
-        v->type_int(v, &value, name, errp);
-        if (value < 0 || value > UINT32_MAX) {
+        v->type_uint64(v, &value, name, errp);
+        if (value > UINT32_MAX) {
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                        name ? name : "null", "uint32_t");
             return;
@@ -156,15 +156,7 @@ void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, Error **errp

 void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
 {
-    int64_t value;
-
-    if (v->type_uint64) {
-        v->type_uint64(v, obj, name, errp);
-    } else {
-        value = *obj;
-        v->type_int(v, &value, name, errp);
-        *obj = value;
-    }
+    v->type_uint64(v, obj, name, errp);
 }

 void visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp)
@@ -175,7 +167,7 @@ void visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp)
         v->type_int8(v, obj, name, errp);
     } else {
         value = *obj;
-        v->type_int(v, &value, name, errp);
+        v->type_int64(v, &value, name, errp);
         if (value < INT8_MIN || value > INT8_MAX) {
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                        name ? name : "null", "int8_t");
@@ -193,7 +185,7 @@ void visit_type_int16(Visitor *v, int16_t *obj, const char *name, Error **errp)
         v->type_int16(v, obj, name, errp);
     } else {
         value = *obj;
-        v->type_int(v, &value, name, errp);
+        v->type_int64(v, &value, name, errp);
         if (value < INT16_MIN || value > INT16_MAX) {
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                        name ? name : "null", "int16_t");
@@ -211,7 +203,7 @@ void visit_type_int32(Visitor *v, int32_t *obj, const char *name, Error **errp)
         v->type_int32(v, obj, name, errp);
     } else {
         value = *obj;
-        v->type_int(v, &value, name, errp);
+        v->type_int64(v, &value, name, errp);
         if (value < INT32_MIN || value > INT32_MAX) {
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                        name ? name : "null", "int32_t");
@@ -223,25 +215,15 @@ void visit_type_int32(Visitor *v, int32_t *obj, const char *name, Error **errp)

 void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp)
 {
-    if (v->type_int64) {
-        v->type_int64(v, obj, name, errp);
-    } else {
-        v->type_int(v, obj, name, errp);
-    }
+    v->type_int64(v, obj, name, errp);
 }

 void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
 {
-    int64_t value;
-
     if (v->type_size) {
         v->type_size(v, obj, name, errp);
-    } else if (v->type_uint64) {
-        v->type_uint64(v, obj, name, errp);
     } else {
-        value = *obj;
-        v->type_int(v, &value, name, errp);
-        *obj = value;
+        v->type_uint64(v, obj, name, errp);
     }
 }

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 96dafcb..ba743db 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -356,7 +356,6 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
     v->visitor.next_list = qmp_input_next_list;
     v->visitor.end_list = qmp_input_end_list;
     v->visitor.type_enum = input_type_enum;
-    v->visitor.type_int = qmp_input_type_int64;
     v->visitor.type_int64 = qmp_input_type_int64;
     v->visitor.type_uint64 = qmp_input_type_uint64;
     v->visitor.type_bool = qmp_input_type_bool;
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index a52f26f..e66ab3c 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -248,7 +248,6 @@ QmpOutputVisitor *qmp_output_visitor_new(void)
     v->visitor.next_list = qmp_output_next_list;
     v->visitor.end_list = qmp_output_end_list;
     v->visitor.type_enum = output_type_enum;
-    v->visitor.type_int = qmp_output_type_int64;
     v->visitor.type_int64 = qmp_output_type_int64;
     v->visitor.type_uint64 = qmp_output_type_uint64;
     v->visitor.type_bool = qmp_output_type_bool;
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 0931321..0a7ddee 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -344,7 +344,6 @@ StringInputVisitor *string_input_visitor_new(const char *str)
     v = g_malloc0(sizeof(*v));

     v->visitor.type_enum = input_type_enum;
-    v->visitor.type_int = parse_type_int64;
     v->visitor.type_int64 = parse_type_int64;
     v->visitor.type_uint64 = parse_type_uint64;
     v->visitor.type_size = parse_type_size;
diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index 83ca6cc..bcd72bb 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -348,7 +348,6 @@ StringOutputVisitor *string_output_visitor_new(bool human)
     v->string = g_string_new(NULL);
     v->human = human;
     v->visitor.type_enum = output_type_enum;
-    v->visitor.type_int = print_type_int64;
     v->visitor.type_int64 = print_type_int64;
     v->visitor.type_uint64 = print_type_uint64;
     v->visitor.type_size = print_type_size;
-- 
2.4.3

[Qemu-devel] [PATCH v6 03/23] qapi: Consolidate visitor integer callbacks

[Eric Blake]

Commit 4e27e819 introduced optional visitor callbacks for all
sorts of int types, but except for type_uint64() and type_size(),
none of them have ever been supplied (the generic implementation
based on using type_[u]int64() then bounds-checking works just
fine). In the interest of simplicity, it's easier to make the
visitor callback interface not have to worry about the other
sizes.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: split off from v5 23/46
original version also appeared in v6-v9 of subset D
---
 include/qapi/visitor-impl.h |  22 ++++-----
 qapi/qapi-visit-core.c      | 117 ++++++++++++++++++--------------------------
 2 files changed, 59 insertions(+), 80 deletions(-)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 70326e0..d071c06 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -1,7 +1,7 @@
 /*
  * Core Definitions for QAPI Visitor implementations
  *
- * Copyright (C) 2012 Red Hat, Inc.
+ * Copyright (C) 2012, 2015 Red Hat, Inc.
  *
  * Author: Paolo Bonizni <pbonzini@redhat.com>
  *
@@ -36,6 +36,16 @@ struct Visitor
     void (*get_next_type)(Visitor *v, QType *type, bool promote_int,
                           const char *name, Error **errp);

+    /* Must be set. */
+    void (*type_int64)(Visitor *v, int64_t *obj, const char *name,
+                       Error **errp);
+    /* Must be set. */
+    void (*type_uint64)(Visitor *v, uint64_t *obj, const char *name,
+                        Error **errp);
+    /* Only required to visit sizes differently than (*type_uint64)().  */
+    void (*type_size)(Visitor *v, uint64_t *obj, const char *name,
+                      Error **errp);
+    /* Must be set. */
     void (*type_bool)(Visitor *v, bool *obj, const char *name, Error **errp);
     void (*type_str)(Visitor *v, char **obj, const char *name, Error **errp);
     void (*type_number)(Visitor *v, double *obj, const char *name,
@@ -46,16 +56,6 @@ struct Visitor
     /* May be NULL; most useful for input visitors. */
     void (*optional)(Visitor *v, bool *present, const char *name);

-    void (*type_uint8)(Visitor *v, uint8_t *obj, const char *name, Error **errp);
-    void (*type_uint16)(Visitor *v, uint16_t *obj, const char *name, Error **errp);
-    void (*type_uint32)(Visitor *v, uint32_t *obj, const char *name, Error **errp);
-    void (*type_uint64)(Visitor *v, uint64_t *obj, const char *name, Error **errp);
-    void (*type_int8)(Visitor *v, int8_t *obj, const char *name, Error **errp);
-    void (*type_int16)(Visitor *v, int16_t *obj, const char *name, Error **errp);
-    void (*type_int32)(Visitor *v, int32_t *obj, const char *name, Error **errp);
-    void (*type_int64)(Visitor *v, int64_t *obj, const char *name, Error **errp);
-    /* visit_type_size() falls back to (*type_uint64)() if type_size is unset */
-    void (*type_size)(Visitor *v, uint64_t *obj, const char *name, Error **errp);
     bool (*start_union)(Visitor *v, bool data_present, Error **errp);
     void (*end_union)(Visitor *v, bool data_present, Error **errp);
 };
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 88bed9c..be1fcdd 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -104,57 +104,48 @@ void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp)
 {
     uint64_t value;

-    if (v->type_uint8) {
-        v->type_uint8(v, obj, name, errp);
-    } else {
-        value = *obj;
-        v->type_uint64(v, &value, name, errp);
-        if (value > UINT8_MAX) {
-            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-                       name ? name : "null", "uint8_t");
-            return;
-        }
-        *obj = value;
+    value = *obj;
+    v->type_uint64(v, &value, name, errp);
+    if (value > UINT8_MAX) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   name ? name : "null", "uint8_t");
+        return;
     }
+    *obj = value;
 }

-void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Error **errp)
+void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name,
+                       Error **errp)
 {
     uint64_t value;

-    if (v->type_uint16) {
-        v->type_uint16(v, obj, name, errp);
-    } else {
-        value = *obj;
-        v->type_uint64(v, &value, name, errp);
-        if (value > UINT16_MAX) {
-            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-                       name ? name : "null", "uint16_t");
-            return;
-        }
-        *obj = value;
+    value = *obj;
+    v->type_uint64(v, &value, name, errp);
+    if (value > UINT16_MAX) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   name ? name : "null", "uint16_t");
+        return;
     }
+    *obj = value;
 }

-void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, Error **errp)
+void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name,
+                       Error **errp)
 {
     uint64_t value;

-    if (v->type_uint32) {
-        v->type_uint32(v, obj, name, errp);
-    } else {
-        value = *obj;
-        v->type_uint64(v, &value, name, errp);
-        if (value > UINT32_MAX) {
-            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-                       name ? name : "null", "uint32_t");
-            return;
-        }
-        *obj = value;
+    value = *obj;
+    v->type_uint64(v, &value, name, errp);
+    if (value > UINT32_MAX) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   name ? name : "null", "uint32_t");
+        return;
     }
+    *obj = value;
 }

-void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
+void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name,
+                       Error **errp)
 {
     v->type_uint64(v, obj, name, errp);
 }
@@ -163,54 +154,42 @@ void visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp)
 {
     int64_t value;

-    if (v->type_int8) {
-        v->type_int8(v, obj, name, errp);
-    } else {
-        value = *obj;
-        v->type_int64(v, &value, name, errp);
-        if (value < INT8_MIN || value > INT8_MAX) {
-            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-                       name ? name : "null", "int8_t");
-            return;
-        }
-        *obj = value;
+    value = *obj;
+    v->type_int64(v, &value, name, errp);
+    if (value < INT8_MIN || value > INT8_MAX) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   name ? name : "null", "int8_t");
+        return;
     }
+    *obj = value;
 }

 void visit_type_int16(Visitor *v, int16_t *obj, const char *name, Error **errp)
 {
     int64_t value;

-    if (v->type_int16) {
-        v->type_int16(v, obj, name, errp);
-    } else {
-        value = *obj;
-        v->type_int64(v, &value, name, errp);
-        if (value < INT16_MIN || value > INT16_MAX) {
-            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-                       name ? name : "null", "int16_t");
-            return;
-        }
-        *obj = value;
+    value = *obj;
+    v->type_int64(v, &value, name, errp);
+    if (value < INT16_MIN || value > INT16_MAX) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   name ? name : "null", "int16_t");
+        return;
     }
+    *obj = value;
 }

 void visit_type_int32(Visitor *v, int32_t *obj, const char *name, Error **errp)
 {
     int64_t value;

-    if (v->type_int32) {
-        v->type_int32(v, obj, name, errp);
-    } else {
-        value = *obj;
-        v->type_int64(v, &value, name, errp);
-        if (value < INT32_MIN || value > INT32_MAX) {
-            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-                       name ? name : "null", "int32_t");
-            return;
-        }
-        *obj = value;
+    value = *obj;
+    v->type_int64(v, &value, name, errp);
+    if (value < INT32_MIN || value > INT32_MAX) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   name ? name : "null", "int32_t");
+        return;
     }
+    *obj = value;
 }

 void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp)
-- 
2.4.3

[Qemu-devel] [PATCH v6 04/23] qapi: Don't cast Enum* to int*

[Eric Blake]

C compilers are allowed to represent enums as a smaller type
than int, if all enum values fit in the smaller type.  There
are even compiler flags that force the use of this smaller
representation, and using them changes the ABI of a binary.
Therefore, our generated code for visit_type_ENUM() (for all
qapi enums) was wrong for casting Enum* to int* when calling
visit_type_enum().

It appears that no one has been doing this for qemu, because
if they had, we are potentially dereferencing beyond bounds
or even risking a SIGBUS on platforms where unaligned pointer
dereferencing is fatal.  Better is to avoid the practice
entirely, and just use the correct types.

This matches the fix for alternate qapi types, earlier in
"qapi: Simplify visiting of alternate types".

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: new patch
---
 scripts/qapi-visit.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index dc2a336..ddfb769 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -172,12 +172,13 @@ out:


 def gen_visit_enum(name):
-    # FIXME cast from enum *obj to int * invalidely assumes enum is int
     return mcgen('''

 void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj, const char *name, Error **errp)
 {
-    visit_type_enum(v, (int *)obj, %(c_name)s_lookup, "%(name)s", name, errp);
+    int tmp = *obj;
+    visit_type_enum(v, &tmp, %(c_name)s_lookup, "%(name)s", name, errp);
+    *obj = tmp;
 }
 ''',
                  c_name=c_name(name), name=name)
-- 
2.4.3

[Qemu-devel] [PATCH v6 05/23] qmp: Fix reference-counting of qnull on empty output visit

[Eric Blake]

Commit 6c2f9a15 ensured that we would not return NULL when the
caller used an output visitor but had nothing to visit. But
in doing so, it added a FIXME about a reference count leak
that could abort qemu in the (unlikely) case of SIZE_MAX such
visits (more plausible on 32-bit).

This fixes things by documenting the internal contracts, and
explaining why the internal function can return NULL and only
the public facing interface needs to worry about qnull(),
thus avoiding over-referencing the qnull_ global object.

It does not, however, fix the stupidity of the stack mixing
up two separate pieces of information; add a FIXME to explain
that issue.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: no change
---
 qapi/qmp-output-visitor.c       | 30 ++++++++++++++++++++++++++++--
 tests/test-qmp-output-visitor.c |  2 ++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index e66ab3c..9d0f9d1 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -29,6 +29,12 @@ typedef QTAILQ_HEAD(QStack, QStackEntry) QStack;
 struct QmpOutputVisitor
 {
     Visitor visitor;
+    /* FIXME: we are abusing stack to hold two separate pieces of
+     * information: the current root object, and the stack of objects
+     * still being built.  Worse, our behavior is inconsistent:
+     * visiting two top-level scalars in a row discards the first in
+     * favor of the second, but visiting two top-level objects in a
+     * row tries to append the second object into the first.  */
     QStack stack;
 };

@@ -41,10 +47,12 @@ static QmpOutputVisitor *to_qov(Visitor *v)
     return container_of(v, QmpOutputVisitor, visitor);
 }

+/* Push @value onto the stack of current QObjects being built */
 static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value)
 {
     QStackEntry *e = g_malloc0(sizeof(*e));

+    assert(value);
     e->value = value;
     if (qobject_type(e->value) == QTYPE_QLIST) {
         e->is_list_head = true;
@@ -52,16 +60,20 @@ static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value)
     QTAILQ_INSERT_HEAD(&qov->stack, e, node);
 }

+/* Grab and remove the most recent QObject from the stack */
 static QObject *qmp_output_pop(QmpOutputVisitor *qov)
 {
     QStackEntry *e = QTAILQ_FIRST(&qov->stack);
     QObject *value;
+
+    assert(e);
     QTAILQ_REMOVE(&qov->stack, e, node);
     value = e->value;
     g_free(e);
     return value;
 }

+/* Grab the root QObject, if any, in preparation to empty the stack */
 static QObject *qmp_output_first(QmpOutputVisitor *qov)
 {
     QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack);
@@ -72,24 +84,32 @@ static QObject *qmp_output_first(QmpOutputVisitor *qov)
      * handle null.
      */
     if (!e) {
-        return qnull();
+        /* No root */
+        return NULL;
     }
-
+    assert(e->value);
     return e->value;
 }

+/* Grab the most recent QObject from the stack, which must exist */
 static QObject *qmp_output_last(QmpOutputVisitor *qov)
 {
     QStackEntry *e = QTAILQ_FIRST(&qov->stack);
+
+    assert(e);
     return e->value;
 }

+/* Add @value to the current QObject being built.
+ * If the stack is visiting a dictionary or list, @value is now owned
+ * by that container. Otherwise, @value is now the root.  */
 static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
                                QObject *value)
 {
     QObject *cur;

     if (QTAILQ_EMPTY(&qov->stack)) {
+        /* Stack was empty, track this object as root */
         qmp_output_push_obj(qov, value);
         return;
     }
@@ -98,13 +118,16 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,

     switch (qobject_type(cur)) {
     case QTYPE_QDICT:
+        assert(name);
         qdict_put_obj(qobject_to_qdict(cur), name, value);
         break;
     case QTYPE_QLIST:
         qlist_append_obj(qobject_to_qlist(cur), value);
         break;
     default:
+        /* The previous root was a scalar, replace it with a new root */
         qobject_decref(qmp_output_pop(qov));
+        assert(QTAILQ_EMPTY(&qov->stack));
         qmp_output_push_obj(qov, value);
         break;
     }
@@ -205,11 +228,14 @@ static void qmp_output_type_any(Visitor *v, QObject **obj, const char *name,
     qmp_output_add_obj(qov, name, *obj);
 }

+/* Finish building, and return the root object. Will not be NULL. */
 QObject *qmp_output_get_qobject(QmpOutputVisitor *qov)
 {
     QObject *obj = qmp_output_first(qov);
     if (obj) {
         qobject_incref(obj);
+    } else {
+        obj = qnull();
     }
     return obj;
 }
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 3078442..8e6fc33 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -461,6 +461,8 @@ static void test_visitor_out_empty(TestOutputVisitorData *data,

     arg = qmp_output_get_qobject(data->qov);
     g_assert(qobject_type(arg) == QTYPE_QNULL);
+    /* Check that qnull reference counting is sane */
+    g_assert(arg->refcnt == 2);
     qobject_decref(arg);
 }

-- 
2.4.3

[Qemu-devel] [PATCH v6 06/23] qapi: Don't abuse stack to track qmp-output root

[Eric Blake]

The previous commit documented an inconsistency in how we are
using the stack of qmp-output-visitor.  Normally, pushing a
single top-level object puts the object on the stack twice:
once as the root, and once as the current container being
appended to; but popping that struct only pops once.  However,
qmp_ouput_add() was trying to either set up the added object
as the new root (works if you parse two top-level scalars in a
row: the second replaces the first as the root) or as a member
of the current container (works as long as you have an open
container on the stack; but if you have popped the first
top-level container, it then resolves to the root and still
tries to add into that existing container).

Fix the stupidity by not tracking two separate things in the
stack.

Not done here: maybe qmp_output_get_object() should assert that
the stack is empty, rather than letting users look at the current
root even while the root is still being visited.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: no change
---
 qapi/qmp-output-visitor.c | 70 +++++++++++++++--------------------------------
 1 file changed, 22 insertions(+), 48 deletions(-)

diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index 9d0f9d1..4a28ce3 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -29,13 +29,8 @@ typedef QTAILQ_HEAD(QStack, QStackEntry) QStack;
 struct QmpOutputVisitor
 {
     Visitor visitor;
-    /* FIXME: we are abusing stack to hold two separate pieces of
-     * information: the current root object, and the stack of objects
-     * still being built.  Worse, our behavior is inconsistent:
-     * visiting two top-level scalars in a row discards the first in
-     * favor of the second, but visiting two top-level objects in a
-     * row tries to append the second object into the first.  */
-    QStack stack;
+    QStack stack; /* Stack of containers still growing */
+    QObject *root; /* Root of the output visit */
 };

 #define qmp_output_add(qov, name, value) \
@@ -52,6 +47,7 @@ static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value)
 {
     QStackEntry *e = g_malloc0(sizeof(*e));

+    assert(qov->root);
     assert(value);
     e->value = value;
     if (qobject_type(e->value) == QTYPE_QLIST) {
@@ -76,28 +72,15 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov)
 /* Grab the root QObject, if any, in preparation to empty the stack */
 static QObject *qmp_output_first(QmpOutputVisitor *qov)
 {
-    QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack);
-
-    /*
-     * FIXME Wrong, because qmp_output_get_qobject() will increment
-     * the refcnt *again*.  We need to think through how visitors
-     * handle null.
-     */
-    if (!e) {
-        /* No root */
-        return NULL;
-    }
-    assert(e->value);
-    return e->value;
+    return qov->root;
 }

-/* Grab the most recent QObject from the stack, which must exist */
+/* Grab the most recent QObject from the stack, if any */
 static QObject *qmp_output_last(QmpOutputVisitor *qov)
 {
     QStackEntry *e = QTAILQ_FIRST(&qov->stack);

-    assert(e);
-    return e->value;
+    return e ? e->value : NULL;
 }

 /* Add @value to the current QObject being built.
@@ -108,28 +91,23 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
 {
     QObject *cur;

-    if (QTAILQ_EMPTY(&qov->stack)) {
-        /* Stack was empty, track this object as root */
-        qmp_output_push_obj(qov, value);
-        return;
-    }
-
     cur = qmp_output_last(qov);

-    switch (qobject_type(cur)) {
-    case QTYPE_QDICT:
-        assert(name);
-        qdict_put_obj(qobject_to_qdict(cur), name, value);
-        break;
-    case QTYPE_QLIST:
-        qlist_append_obj(qobject_to_qlist(cur), value);
-        break;
-    default:
-        /* The previous root was a scalar, replace it with a new root */
-        qobject_decref(qmp_output_pop(qov));
-        assert(QTAILQ_EMPTY(&qov->stack));
-        qmp_output_push_obj(qov, value);
-        break;
+    if (!cur) {
+        qobject_decref(qov->root);
+        qov->root = value;
+    } else {
+        switch (qobject_type(cur)) {
+        case QTYPE_QDICT:
+            assert(name);
+            qdict_put_obj(qobject_to_qdict(cur), name, value);
+            break;
+        case QTYPE_QLIST:
+            qlist_append_obj(qobject_to_qlist(cur), value);
+            break;
+        default:
+            g_assert_not_reached();
+        }
     }
 }

@@ -249,16 +227,12 @@ void qmp_output_visitor_cleanup(QmpOutputVisitor *v)
 {
     QStackEntry *e, *tmp;

-    /* The bottom QStackEntry, if any, owns the root QObject. See the
-     * qmp_output_push_obj() invocations in qmp_output_add_obj(). */
-    QObject *root = QTAILQ_EMPTY(&v->stack) ? NULL : qmp_output_first(v);
-
     QTAILQ_FOREACH_SAFE(e, &v->stack, node, tmp) {
         QTAILQ_REMOVE(&v->stack, e, node);
         g_free(e);
     }

-    qobject_decref(root);
+    qobject_decref(v->root);
     g_free(v);
 }

-- 
2.4.3

[Qemu-devel] [PATCH v6 07/23] qapi: Document visitor interfaces

[Eric Blake]

The visitor interface for mapping between QObject/QemuOpts/string
and qapi has formerly been documented only by reading source code,
making it difficult to propose changes to either scripts/qapi*.py
or to clients without knowing whether those changes would be safe.
This adds documentation, including mentioning when parameters can
be NULL, and where there are still some interface warts that would
be nice to remove.  In particular, I have plans to remove
visit_start_union() in a future patch.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: mention that input visitors blindly assign over *obj; wording
improvements
---
 include/qapi/visitor-impl.h |  33 +++++++-
 include/qapi/visitor.h      | 192 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 215 insertions(+), 10 deletions(-)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index d071c06..2f4c89b 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -15,21 +15,37 @@
 #include "qapi/error.h"
 #include "qapi/visitor.h"

+/* This file describes the callback interface for implementing a
+ * QObject visitor.  For the client interface, see visitor.h.  When
+ * implementing the callbacks, it is easiest to declare a struct with
+ * 'Visitor visitor;' as the first member.  Semantics for the
+ * callbacks are generally similar to the counterpart public
+ * interface.  */
+
 struct Visitor
 {
-    /* Must be set */
+    /* Must be provided to visit structs (the string visitors do not
+     * currently visit structs). */
     void (*start_struct)(Visitor *v, void **obj, const char *kind,
                          const char *name, size_t size, Error **errp);
+    /* Must be provided if start_struct is present. */
     void (*end_struct)(Visitor *v, Error **errp);

+    /* May be NULL; most useful for input visitors. */
     void (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
                                   Error **errp);
+    /* May be NULL */
     void (*end_implicit_struct)(Visitor *v, Error **errp);

+    /* Must be set */
     void (*start_list)(Visitor *v, const char *name, Error **errp);
+    /* Must be set */
     GenericList *(*next_list)(Visitor *v, GenericList **list, Error **errp);
+    /* Must be set */
     void (*end_list)(Visitor *v, Error **errp);

+    /* Must be set, although the helpers input_type_enum() and
+     * output_type_enum() can be used.  */
     void (*type_enum)(Visitor *v, int *obj, const char * const strings[],
                       const char *kind, const char *name, Error **errp);
     /* May be NULL; only needed for input visitors. */
@@ -45,23 +61,38 @@ struct Visitor
     /* Only required to visit sizes differently than (*type_uint64)().  */
     void (*type_size)(Visitor *v, uint64_t *obj, const char *name,
                       Error **errp);
+
     /* Must be set. */
     void (*type_bool)(Visitor *v, bool *obj, const char *name, Error **errp);
+    /* Must be set */
     void (*type_str)(Visitor *v, char **obj, const char *name, Error **errp);
+
+    /* Must be provided to visit numbers (the opts visitor does not
+     * currently visit non-integers). */
     void (*type_number)(Visitor *v, double *obj, const char *name,
                         Error **errp);
+    /* Must be provided to visit arbitrary QTypes (the opts and string
+     * visitors do not currently visit arbitrary types).  */
     void (*type_any)(Visitor *v, QObject **obj, const char *name,
                      Error **errp);

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

+    /* FIXME - needs to be removed */
     bool (*start_union)(Visitor *v, bool data_present, Error **errp);
+    /* FIXME - needs to be removed */
     void (*end_union)(Visitor *v, bool data_present, Error **errp);
 };

+/**
+ * A generic visitor.type_enum suitable for input visitors.
+ */
 void input_type_enum(Visitor *v, int *obj, const char * const strings[],
                      const char *kind, const char *name, Error **errp);
+/**
+ * A generic visitor.type_enum suitable for output visitors.
+ */
 void output_type_enum(Visitor *v, int *obj, const char * const strings[],
                       const char *kind, const char *name, Error **errp);

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index a14a16d..74159c6 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -18,6 +18,20 @@
 #include "qapi/error.h"
 #include <stdlib.h>

+/* This file describes the client view for visiting a map between
+ * generated QAPI C structs and another representation (command line
+ * options, strings, or QObjects).  An input visitor converts from
+ * some other form into QAPI representation; an output visitor
+ * converts from QAPI back into another form.  In the descriptions
+ * below, an object or dictionary refers to a JSON '{}', and a list
+ * refers to a JSON '[]'.  These functions seldom need to be called
+ * directly, but are instead used by code generated by
+ * scripts/qapi-visit.py.  For the visitor callback contracts, see
+ * visitor-impl.h.
+ */
+
+/* This struct is layout-compatible with all other *List structs
+ * created by the qapi generator. */
 typedef struct GenericList
 {
     union {
@@ -27,14 +41,70 @@ typedef struct GenericList
     struct GenericList *next;
 } GenericList;

+/**
+ * Prepare to visit an object with C type @kind tied to key @name.
+ * @name will be NULL if this is visited as part of a list.
+ * The caller then makes a series of visit calls for each key expected
+ * in the object, followed by a call to visit_end_struct(). For an
+ * input visitor, @obj can be NULL to validate that the visit will
+ * succeed; otherwise, *@obj is assigned with an allocation of @size
+ * bytes, without regards to the previous value of *@obj. For other
+ * visitors, *@obj is the object to visit. Set *@errp on failure.
+ *
+ * FIXME: *@obj can be modified even on error; this can lead to
+ * memory leaks if clients aren't careful.
+ */
 void visit_start_struct(Visitor *v, void **obj, const char *kind,
                         const char *name, size_t size, Error **errp);
+/**
+ * Complete a struct visit started earlier.
+ * Must be called after any successful use of visit_start_struct(),
+ * even if intermediate processing was skipped due to errors.
+ */
 void visit_end_struct(Visitor *v, Error **errp);
+
+/**
+ * Prepare to visit an implicit struct.
+ * Similar to visit_start_struct(), except that in implicit struct
+ * represents a subset of keys that are present at the same nesting level
+ * of a common object, rather than a new object at a deeper nesting level.
+ *
+ * FIXME: *@obj can be modified even on error; this can lead to
+ * memory leaks if clients aren't careful.
+ */
 void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
                                  Error **errp);
+/**
+ * Complete an implicit struct visit started earlier.
+ * Must be called after any successful use of visit_start_implicit_struct(),
+ * even if intermediate processing was skipped due to errors.
+ */
 void visit_end_implicit_struct(Visitor *v, Error **errp);
+
+/**
+ * Prepare to visit a list tied to an object key @name.
+ * @name will be NULL if this is visited as part of another list.
+ * After calling this, the elements must be collected until
+ * visit_next_list() returns NULL, then visit_end_list() must be
+ * used to complete the visit.
+ */
 void visit_start_list(Visitor *v, const char *name, Error **errp);
+/**
+ * Collect the next list member and append it to *@list.
+ * Start with *@list of NULL, then subsequent iterations should pass
+ * *@list pointing to the previous return value.  Must be called in a
+ * loop until a NULL return or error occurs; for each non-NULL return,
+ * the caller must then call the appropriate visit_type_*() for the
+ * element type of the list, with that function's name parameter set
+ * to NULL.
+ */
 GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
+/**
+ * Complete the list started earlier.
+ * Must be called after any successful use of visit_start_list(),
+ * even if intermediate processing was skipped due to errors, to allow
+ * the backend to release any resources.
+ */
 void visit_end_list(Visitor *v, Error **errp);

 /**
@@ -53,23 +123,127 @@ bool visit_optional(Visitor *v, bool *present, const char *name);
  */
 void visit_get_next_type(Visitor *v, QType *type, bool promote_int,
                          const char *name, Error **errp);
+
+/**
+ * Visit an enum value tied to @name in the current object visit.
+ * @name will be NULL if this is visited as part of a list.
+ * For input visitors, parse a string and set *@obj to the numeric value
+ * of the enum type @kind using @strings as the mapping, leaving @obj
+ * unchanged on error; for output visitors, reverse the mapping and visit
+ * the output string determined by *@obj.
+ */
 void visit_type_enum(Visitor *v, int *obj, const char * const strings[],
                      const char *kind, const char *name, Error **errp);
+
+/**
+ * Visit an integer value tied to @name in the current object visit.
+ * @name will be NULL if this is visited as part of a list.
+ * For input visitors, set *@obj to the parsed value; for other visitors,
+ * leave *@obj unchanged.
+ */
 void visit_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp);
-void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp);
-void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Error **errp);
-void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, Error **errp);
-void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp);
+/**
+ * Visit a uint8_t value tied to @name in the current object visit.
+ * Like visit_type_int(), except clamps the value to uint8_t range.
+ */
+void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name,
+                      Error **errp);
+/**
+ * Visit a uint16_t value tied to @name in the current object visit.
+ * Like visit_type_int(), except clamps the value to uint16_t range.
+ */
+void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name,
+                       Error **errp);
+/**
+ * Visit a uint32_t value tied to @name in the current object visit.
+ * Like visit_type_int(), except clamps the value to uint32_t range.
+ */
+void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name,
+                       Error **errp);
+/**
+ * Visit a uint64_t value tied to @name in the current object visit.
+ * Like visit_type_int(), except clamps the value to uint64_t range
+ * (that is, ensures it is unsigned).
+ */
+void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name,
+                       Error **errp);
+/**
+ * Visit an int8_t value tied to @name in the current object visit.
+ * Like visit_type_int(), except clamps the value to int8_t range.
+ */
 void visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp);
-void visit_type_int16(Visitor *v, int16_t *obj, const char *name, Error **errp);
-void visit_type_int32(Visitor *v, int32_t *obj, const char *name, Error **errp);
-void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp);
-void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp);
+/**
+ * Visit an int16_t value tied to @name in the current object visit.
+ * Like visit_type_int(), except clamps the value to int16_t range.
+ */
+void visit_type_int16(Visitor *v, int16_t *obj, const char *name,
+                      Error **errp);
+/**
+ * Visit an uint32_t value tied to @name in the current object visit.
+ * Like visit_type_int(), except clamps the value to int32_t range.
+ */
+void visit_type_int32(Visitor *v, int32_t *obj, const char *name,
+                      Error **errp);
+/**
+ * Visit an int64_t value tied to @name in the current object visit.
+ * Like visit_type_int(), except clamps the value to int64_t range.
+ */
+void visit_type_int64(Visitor *v, int64_t *obj, const char *name,
+                      Error **errp);
+/**
+ * Visit a uint64_t value tied to @name in the current object visit.
+ * Like visit_type_uint64(), except that some visitors may choose to
+ * recognize additional suffixes for easily scaling input values.
+ */
+void visit_type_size(Visitor *v, uint64_t *obj, const char *name,
+                     Error **errp);
+
+/**
+ * Visit a boolean value tied to @name in the current object visit.
+ * @name will be NULL if this is visited as part of a list.
+ * Input visitors set *@obj to the value; other visitors will leave
+ * *@obj unchanged.
+ */
 void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp);
+
+/**
+ * Visit a string value tied to @name in the current object visit.
+ * @name will be NULL if this is visited as part of a list.
+ * @obj must be non-NULL.  Input visitors set *@obj to the parsed string;
+ * while output visitors leave *@obj unchanged, except that a NULL *@obj
+ * must be treated the same as "".
+ *
+ * FIXME: Unfortunately not const-correct for output visitors.
+ */
 void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp);
-void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp);
+
+/**
+ * Visit a number value tied to @name in the current object visit.
+ * @name will be NULL if this is visited as part of a list.
+ * Input visitors set *@obj to the value; other visitors will leave
+ * *@obj unchanged.
+ */
+void visit_type_number(Visitor *v, double *obj, const char *name,
+                       Error **errp);
+
+/**
+ * Visit an arbitrary qtype value tied to @name in the current object visit.
+ * @name will be NULL if this is visited as part of a list.
+ * Input visitors set *@obj to the value; other visitors will leave
+ * *@obj unchanged.
+ */
 void visit_type_any(Visitor *v, QObject **obj, const char *name, Error **errp);
+
+/**
+ * Mark the start of visiting the branches of a union. Return true if
+ * @data_present.
+ * FIXME: Should not be needed
+ */
 bool visit_start_union(Visitor *v, bool data_present, Error **errp);
+/**
+ * Mark the end of union branches, after visit_start_union().
+ * FIXME: Should not be needed
+ */
 void visit_end_union(Visitor *v, bool data_present, Error **errp);

 #endif
-- 
2.4.3

[Qemu-devel] [PATCH v6 08/23] qapi: Drop unused error argument for list and implicit struct

[Eric Blake]

No backend was setting an error when ending the visit of a list
or implicit struct.  Make the callers a bit easier to follow by
making this a part of the contract, and removing the errp
argument - callers can then unconditionally end an object as
part of cleanup without having to think about whether a second
error is dominated by a first, because there is no second error.

A later patch will then tackle the larger task of splitting
visit_end_struct(), which can indeed set an error.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: new patch, split from RFC on v5 7/46
---
 hw/ppc/spapr_drc.c           |  2 +-
 include/qapi/visitor-impl.h  |  4 ++--
 include/qapi/visitor.h       |  8 +++++---
 qapi/opts-visitor.c          |  2 +-
 qapi/qapi-dealloc-visitor.c  |  4 ++--
 qapi/qapi-visit-core.c       |  8 ++++----
 qapi/qmp-input-visitor.c     |  9 ++-------
 qapi/qmp-output-visitor.c    |  2 +-
 qapi/string-input-visitor.c  |  2 +-
 qapi/string-output-visitor.c |  2 +-
 scripts/qapi-visit.py        | 10 +++-------
 11 files changed, 23 insertions(+), 30 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index f34bc04..1846b4f 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -293,7 +293,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque,
                 visit_type_uint8(v, (uint8_t *)&prop->data[i], NULL, NULL);

             }
-            visit_end_list(v, NULL);
+            visit_end_list(v);
             break;
         }
         default:
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 2f4c89b..36984a7 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -35,14 +35,14 @@ struct Visitor
     void (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
                                   Error **errp);
     /* May be NULL */
-    void (*end_implicit_struct)(Visitor *v, Error **errp);
+    void (*end_implicit_struct)(Visitor *v);

     /* Must be set */
     void (*start_list)(Visitor *v, const char *name, Error **errp);
     /* Must be set */
     GenericList *(*next_list)(Visitor *v, GenericList **list, Error **errp);
     /* Must be set */
-    void (*end_list)(Visitor *v, Error **errp);
+    void (*end_list)(Visitor *v);

     /* Must be set, although the helpers input_type_enum() and
      * output_type_enum() can be used.  */
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 74159c6..b4ed469 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -77,9 +77,11 @@ void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
 /**
  * Complete an implicit struct visit started earlier.
  * Must be called after any successful use of visit_start_implicit_struct(),
- * even if intermediate processing was skipped due to errors.
+ * even if intermediate processing was skipped due to errors.  Unlike
+ * visit_end_struct(), there is no need for an error check; any errors
+ * such as extra input will be detected when ending the overall struct.
  */
-void visit_end_implicit_struct(Visitor *v, Error **errp);
+void visit_end_implicit_struct(Visitor *v);

 /**
  * Prepare to visit a list tied to an object key @name.
@@ -105,7 +107,7 @@ GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
  * even if intermediate processing was skipped due to errors, to allow
  * the backend to release any resources.
  */
-void visit_end_list(Visitor *v, Error **errp);
+void visit_end_list(Visitor *v);

 /**
  * Check if an optional member @name of an object needs visiting.
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 8104e42..37f172d 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -263,7 +263,7 @@ opts_next_list(Visitor *v, GenericList **list, Error **errp)


 static void
-opts_end_list(Visitor *v, Error **errp)
+opts_end_list(Visitor *v)
 {
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);

diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 5133d37..8246070 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -85,7 +85,7 @@ static void qapi_dealloc_start_implicit_struct(Visitor *v,
     qapi_dealloc_push(qov, obj);
 }

-static void qapi_dealloc_end_implicit_struct(Visitor *v, Error **errp)
+static void qapi_dealloc_end_implicit_struct(Visitor *v)
 {
     QapiDeallocVisitor *qov = to_qov(v);
     void **obj = qapi_dealloc_pop(qov);
@@ -121,7 +121,7 @@ static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList **listp,
     return NULL;
 }

-static void qapi_dealloc_end_list(Visitor *v, Error **errp)
+static void qapi_dealloc_end_list(Visitor *v)
 {
     QapiDeallocVisitor *qov = to_qov(v);
     void *obj = qapi_dealloc_pop(qov);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index be1fcdd..477d73a 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -36,10 +36,10 @@ void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
     }
 }

-void visit_end_implicit_struct(Visitor *v, Error **errp)
+void visit_end_implicit_struct(Visitor *v)
 {
     if (v->end_implicit_struct) {
-        v->end_implicit_struct(v, errp);
+        v->end_implicit_struct(v);
     }
 }

@@ -53,9 +53,9 @@ GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp)
     return v->next_list(v, list, errp);
 }

-void visit_end_list(Visitor *v, Error **errp)
+void visit_end_list(Visitor *v)
 {
-    v->end_list(v, errp);
+    v->end_list(v);
 }

 bool visit_start_union(Visitor *v, bool data_present, Error **errp)
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index ba743db..9ff1e75 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -153,10 +153,6 @@ static void qmp_input_start_implicit_struct(Visitor *v, void **obj,
     }
 }

-static void qmp_input_end_implicit_struct(Visitor *v, Error **errp)
-{
-}
-
 static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
@@ -201,11 +197,11 @@ static GenericList *qmp_input_next_list(Visitor *v, GenericList **list,
     return entry;
 }

-static void qmp_input_end_list(Visitor *v, Error **errp)
+static void qmp_input_end_list(Visitor *v)
 {
     QmpInputVisitor *qiv = to_qiv(v);

-    qmp_input_pop(qiv, errp);
+    qmp_input_pop(qiv, &error_abort);
 }

 static void qmp_input_get_next_type(Visitor *v, QType *type, bool promote_int,
@@ -351,7 +347,6 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
     v->visitor.start_struct = qmp_input_start_struct;
     v->visitor.end_struct = qmp_input_end_struct;
     v->visitor.start_implicit_struct = qmp_input_start_implicit_struct;
-    v->visitor.end_implicit_struct = qmp_input_end_implicit_struct;
     v->visitor.start_list = qmp_input_start_list;
     v->visitor.next_list = qmp_input_next_list;
     v->visitor.end_list = qmp_input_end_list;
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index 4a28ce3..8035649 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -153,7 +153,7 @@ static GenericList *qmp_output_next_list(Visitor *v, GenericList **listp,
     return list ? list->next : NULL;
 }

-static void qmp_output_end_list(Visitor *v, Error **errp)
+static void qmp_output_end_list(Visitor *v)
 {
     QmpOutputVisitor *qov = to_qov(v);
     qmp_output_pop(qov);
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 0a7ddee..5b2bc47 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -173,7 +173,7 @@ next_list(Visitor *v, GenericList **list, Error **errp)
 }

 static void
-end_list(Visitor *v, Error **errp)
+end_list(Visitor *v)
 {
     StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
     siv->head = true;
diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index bcd72bb..54d4174 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -298,7 +298,7 @@ next_list(Visitor *v, GenericList **list, Error **errp)
 }

 static void
-end_list(Visitor *v, Error **errp)
+end_list(Visitor *v)
 {
     StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index ddfb769..8c964b5 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -62,7 +62,7 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error *
     visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err);
     if (!err) {
         visit_type_%(c_type)s_fields(v, obj, errp);
-        visit_end_implicit_struct(v, &err);
+        visit_end_implicit_struct(v);
     }
     error_propagate(errp, err);
 }
@@ -161,9 +161,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
         visit_type_%(c_elt_type)s(v, &native_i->value, NULL, &err);
     }

-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_list(v, &err);
+    visit_end_list(v);
 out:
     error_propagate(errp, err);
 }
@@ -224,9 +222,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
                    "%(name)s");
     }
 out_obj:
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_implicit_struct(v, &err);
+    visit_end_implicit_struct(v);
 out:
     error_propagate(errp, err);
 }
-- 
2.4.3

[Qemu-devel] [PATCH v6 09/23] hmp: Improve use of qapi visitor

[Eric Blake]

Cache the visitor in a local variable instead of repeatedly
calling the accessor.  Pass NULL for the visit_start_struct()
object (which matches the fact that we were already passing 0
for the size argument, because we aren't using the visit to
allocate a qapi struct).  Pass "object" for the struct name,
for better error messages. Reflow the logic so that we don't
have to undo an object_add().

A later patch will then split the error detection currently
in visit_struct_end(), at which point we can again hoist the
object_add() to occur before the label as one of the cleanups
enabled by that split.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: new patch, split from RFC on v5 7/46
---
 hmp.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/hmp.c b/hmp.c
index c2b2c16..ec1d682 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1667,9 +1667,9 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
     QemuOpts *opts;
     char *type = NULL;
     char *id = NULL;
-    void *dummy = NULL;
     OptsVisitor *ov;
     QDict *pdict;
+    Visitor *v;

     opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
     if (err) {
@@ -1678,30 +1678,29 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)

     ov = opts_visitor_new(opts);
     pdict = qdict_clone_shallow(qdict);
+    v = opts_get_visitor(ov);

-    visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
+    visit_start_struct(v, NULL, "object", NULL, 0, &err);
     if (err) {
         goto out_clean;
     }

     qdict_del(pdict, "qom-type");
-    visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err);
+    visit_type_str(v, &type, "qom-type", &err);
     if (err) {
         goto out_end;
     }

     qdict_del(pdict, "id");
-    visit_type_str(opts_get_visitor(ov), &id, "id", &err);
+    visit_type_str(v, &id, "id", &err);
     if (err) {
         goto out_end;
     }

-    object_add(type, id, pdict, opts_get_visitor(ov), &err);
-
 out_end:
-    visit_end_struct(opts_get_visitor(ov), &err_end);
-    if (!err && err_end) {
-        qmp_object_del(id, NULL);
+    visit_end_struct(v, &err_end);
+    if (!err && !err_end) {
+        object_add(type, id, pdict, v, &err);
     }
     error_propagate(&err, err_end);
 out_clean:
@@ -1711,7 +1710,6 @@ out_clean:
     qemu_opts_del(opts);
     g_free(id);
     g_free(type);
-    g_free(dummy);

 out:
     hmp_handle_error(mon, &err);
-- 
2.4.3

[Qemu-devel] [PATCH v6 10/23] vl: Improve use of qapi visitor

[Eric Blake]

Cache the visitor in a local variable instead of repeatedly
calling the accessor.  Pass NULL for the visit_start_struct()
object (which matches the fact that we were already passing 0
for the size argument, because we aren't using the visit to
allocate a qapi struct).  Pass "object" for the struct name,
for better error messages. Reflow the logic so that we don't
have to undo an object_add().

A later patch will then split the error detection currently
in visit_struct_end(), at which point we can again hoist the
object_add() to occur before the label as one of the cleanups
enabled by that split.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: new patch, split from RFC on v5 7/46
---
 vl.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/vl.c b/vl.c
index 4d6263d..4e69815 100644
--- a/vl.c
+++ b/vl.c
@@ -2828,43 +2828,42 @@ static bool object_create_delayed(const char *type)
 static int object_create(void *opaque, QemuOpts *opts, Error **errp)
 {
     Error *err = NULL;
+    Error *err_end = NULL;
     char *type = NULL;
     char *id = NULL;
-    void *dummy = NULL;
     OptsVisitor *ov;
     QDict *pdict;
     bool (*type_predicate)(const char *) = opaque;
+    Visitor *v;

     ov = opts_visitor_new(opts);
     pdict = qemu_opts_to_qdict(opts, NULL);
+    v = opts_get_visitor(ov);

-    visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
+    visit_start_struct(v, NULL, "object", NULL, 0, &err);
     if (err) {
         goto out;
     }

     qdict_del(pdict, "qom-type");
-    visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err);
+    visit_type_str(v, &type, "qom-type", &err);
     if (err) {
         goto out;
     }
     if (!type_predicate(type)) {
-        goto out;
+        goto out_end;
     }

     qdict_del(pdict, "id");
-    visit_type_str(opts_get_visitor(ov), &id, "id", &err);
+    visit_type_str(v, &id, "id", &err);
     if (err) {
-        goto out;
+        goto out_end;
     }

-    object_add(type, id, pdict, opts_get_visitor(ov), &err);
-    if (err) {
-        goto out;
-    }
-    visit_end_struct(opts_get_visitor(ov), &err);
-    if (err) {
-        qmp_object_del(id, NULL);
+out_end:
+    visit_end_struct(v, &err_end);
+    if (!err && !err_end) {
+        object_add(type, id, pdict, v, &err);
     }

 out:
@@ -2873,7 +2872,6 @@ out:
     QDECREF(pdict);
     g_free(id);
     g_free(type);
-    g_free(dummy);
     if (err) {
         error_report_err(err);
         return -1;
-- 
2.4.3

[Qemu-devel] [PATCH v6 11/23] ppc: Improve use of qapi visitors

[Eric Blake]

The implementation of prop_get_fdt() is taking some shortcuts
with the qapi visitor functions.  Document them, and use
error_abort rather than NULL to ensure that any changes to
the visitors do not break our use of shortcuts.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: new patch, split from RFC on v5 7/46
---
 hw/ppc/spapr_drc.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 1846b4f..03a1879 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -276,21 +276,26 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque,
         case FDT_BEGIN_NODE:
             fdt_depth++;
             name = fdt_get_name(fdt, fdt_offset, &name_len);
-            visit_start_struct(v, NULL, NULL, name, 0, NULL);
+            visit_start_struct(v, NULL, "fdt", name, 0, &error_abort);
             break;
         case FDT_END_NODE:
             /* shouldn't ever see an FDT_END_NODE before FDT_BEGIN_NODE */
             g_assert(fdt_depth > 0);
-            visit_end_struct(v, NULL);
+            visit_end_struct(v, &error_abort);
             fdt_depth--;
             break;
         case FDT_PROP: {
             int i;
             prop = fdt_get_property_by_offset(fdt, fdt_offset, &prop_len);
             name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
-            visit_start_list(v, name, NULL);
+            /* Note: since v is an output visitor, we can get away
+             * with just visiting a direct sequence of uint8 into the
+             * output array, instead of creating a uint8List and using
+             * visit_type_uint8List(). */
+            visit_start_list(v, name, &error_abort);
             for (i = 0; i < prop_len; i++) {
-                visit_type_uint8(v, (uint8_t *)&prop->data[i], NULL, NULL);
+                visit_type_uint8(v, (uint8_t *)&prop->data[i], NULL,
+                                 &error_abort);

             }
             visit_end_list(v);
-- 
2.4.3

[Qemu-devel] [PATCH v6 12/23] balloon: Improve use of qapi visitor

[Eric Blake]

Rework the control flow of balloon_stats_get_all() to make it
easier for a later patch to split visit_end_struct().  Also
switch to the uint64 visitor to match the data type.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: new patch, split from RFC on v5 7/46
---
 hw/virtio/virtio-balloon.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 9671635..1ce987a 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -130,10 +130,13 @@ static void balloon_stats_get_all(Object *obj, struct Visitor *v,
     if (err) {
         goto out_end;
     }
-    for (i = 0; !err && i < VIRTIO_BALLOON_S_NR; i++) {
-        visit_type_int64(v, (int64_t *) &s->stats[i], balloon_stat_names[i],
-                         &err);
+    for (i = 0; i < VIRTIO_BALLOON_S_NR; i++) {
+        visit_type_uint64(v, &s->stats[i], balloon_stat_names[i], &err);
+        if (err) {
+            goto out_nested;
+        }
     }
+out_nested:
     error_propagate(errp, err);
     err = NULL;
     visit_end_struct(v, &err);
-- 
2.4.3

[Qemu-devel] [PATCH v6 13/23] qapi: Add type.is_empty() helper

[Eric Blake]

And use it in qapi-types and qapi-event.  Down the road, we may
want to lift our artificial restriction of no variants at the
top level of an event, at which point, inlining our check for
whether members is empty will no longer be sufficient.  More
immediately, the new .is_empty() helper will help fix a bug in
qapi-visit.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: new patch
---
 scripts/qapi-event.py | 6 +++---
 scripts/qapi-types.py | 2 +-
 scripts/qapi.py       | 3 +++
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 720486f..51128f4 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -39,7 +39,7 @@ def gen_event_send(name, arg_type):
 ''',
                 proto=gen_event_send_proto(name, arg_type))

-    if arg_type and arg_type.members:
+    if arg_type and not arg_type.is_empty():
         ret += mcgen('''
     QmpOutputVisitor *qov;
     Visitor *v;
@@ -58,7 +58,7 @@ def gen_event_send(name, arg_type):
 ''',
                  name=name)

-    if arg_type and arg_type.members:
+    if arg_type and not arg_type.is_empty():
         ret += mcgen('''
     qov = qmp_output_visitor_new();
     g_assert(qov);
@@ -90,7 +90,7 @@ def gen_event_send(name, arg_type):
 ''',
                  c_enum=c_enum_const(event_enum_name, name))

-    if arg_type and arg_type.members:
+    if arg_type and not arg_type.is_empty():
         ret += mcgen('''
 out:
     qmp_output_visitor_cleanup(qov);
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index d0e1f74..af6b324 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -76,7 +76,7 @@ struct %(c_name)s {
     # potential issues with attempting to malloc space for zero-length
     # structs in C, and also incompatibility with C++ (where an empty
     # struct is size 1).
-    if not (base and base.members) and not members and not variants:
+    if (not base or base.is_empty()) and not members and not variants:
         ret += mcgen('''
     char qapi_dummy_field_for_empty_struct;
 ''')
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 4197b30..45bc5a7 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -975,6 +975,9 @@ class QAPISchemaObjectType(QAPISchemaType):
         # See QAPISchema._make_implicit_object_type()
         return self.name[0] == ':'

+    def is_empty(self):
+        return not self.members and not self.variants
+
     def c_name(self):
         assert not self.is_implicit()
         return QAPISchemaType.c_name(self)
-- 
2.4.3

[Qemu-devel] [PATCH v6 14/23] qapi: Fix command with named empty argument type

[Eric Blake]

The generator special-cased
 { 'command':'foo', 'data': {} }
to avoid emitting a visitor variable, but failed to see that
 { 'struct':'NamedEmptyType, 'data': {} }
 { 'command':'foo', 'data':'NamedEmptyType' }
needs the same treatment.  Without a fix to the generator, the
change to qapi-schema-test.json fails to compile with:

tests/test-qmp-marshal.c: In function ‘qmp_marshal_user_def_cmd0’:
tests/test-qmp-marshal.c:264:14: error: variable ‘v’ set but not used [-Werror=unused-but-set-variable]
     Visitor *v;
              ^

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: new patch
---
 scripts/qapi-commands.py                | 6 +++---
 tests/qapi-schema/qapi-schema-test.json | 2 ++
 tests/qapi-schema/qapi-schema-test.out  | 2 ++
 tests/test-qmp-commands.c               | 5 +++++
 4 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 561e47a..38cbffc 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -65,7 +65,7 @@ def gen_marshal_vars(arg_type, ret_type):
 ''',
                      c_type=ret_type.c_type())

-    if arg_type:
+    if arg_type and not arg_type.is_empty():
         ret += mcgen('''
     QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
     QapiDeallocVisitor *qdv;
@@ -97,7 +97,7 @@ def gen_marshal_vars(arg_type, ret_type):
 def gen_marshal_input_visit(arg_type, dealloc=False):
     ret = ''

-    if not arg_type:
+    if not arg_type or arg_type.is_empty():
         return ret

     if dealloc:
@@ -177,7 +177,7 @@ def gen_marshal(name, arg_type, ret_type):

     # 'goto out' produced by gen_marshal_input_visit->gen_visit_fields()
     # for each arg_type member, and by gen_call() for ret_type
-    if (arg_type and arg_type.members) or ret_type:
+    if (arg_type and not arg_type.is_empty()) or ret_type:
         ret += mcgen('''

 out:
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 4b89527..a0fdb88 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -18,6 +18,8 @@
 { 'struct': 'Empty1', 'data': { } }
 { 'struct': 'Empty2', 'base': 'Empty1', 'data': { } }

+{ 'command': 'user_def_cmd0', 'data': 'Empty2', 'returns': 'Empty2' }
+
 # for testing override of default naming heuristic
 { 'enum': 'QEnumTwo',
   'prefix': 'QENUM_TWO',
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 2c546b7..d8f9108 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -198,6 +198,8 @@ command guest-sync :obj-guest-sync-arg -> any
    gen=True success_response=True
 command user_def_cmd None -> None
    gen=True success_response=True
+command user_def_cmd0 Empty2 -> Empty2
+   gen=True success_response=True
 command user_def_cmd1 :obj-user_def_cmd1-arg -> None
    gen=True success_response=True
 command user_def_cmd2 :obj-user_def_cmd2-arg -> UserDefTwo
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 9f35b80..b132775 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -12,6 +12,11 @@ void qmp_user_def_cmd(Error **errp)
 {
 }

+Empty2 *qmp_user_def_cmd0(Error **errp)
+{
+    return g_new0(Empty2, 1);
+}
+
 void qmp_user_def_cmd1(UserDefOne * ud1, Error **errp)
 {
 }
-- 
2.4.3

[Qemu-devel] [PATCH v6 15/23] qapi: Improve generated event use of qapi visitor

[Eric Blake]

All other successful clients of visit_start_struct() were paired
with an unconditional visit_end_struct(); but the generated
code for events was relying on qmp_output_visitor_cleanup() to
work on an incomplete visit.  Alter the code to guarantee that
the struct is completed, which will make a future patch to
split visit_end_struct() easier to reason about.  While at it,
drop some assertions and comments that are not present in other
uses of the qmp output visitor, and rearrange the declaration
to make it easier for a future patch to introduce the notion of
a boxed event visit.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: new patch

If desired, I can defer the hunk re-ordering the declaration of
obj to later in the series where it actually comes in handy.
---
 scripts/qapi-event.py | 19 ++++++++++---------
 scripts/qapi.py       |  5 +++--
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 51128f4..5dc9726 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -41,9 +41,9 @@ def gen_event_send(name, arg_type):

     if arg_type and not arg_type.is_empty():
         ret += mcgen('''
+    QObject *obj;
     QmpOutputVisitor *qov;
     Visitor *v;
-    QObject *obj;

 ''')

@@ -59,27 +59,28 @@ def gen_event_send(name, arg_type):
                  name=name)

     if arg_type and not arg_type.is_empty():
+        c_name = 'NULL'
+        if not arg_type.is_implicit():
+            c_name = '"%s"' % arg_type.c_name()
         ret += mcgen('''
     qov = qmp_output_visitor_new();
-    g_assert(qov);
-
     v = qmp_output_get_visitor(qov);
-    g_assert(v);

-    /* Fake visit, as if all members are under a structure */
-    visit_start_struct(v, NULL, "", "%(name)s", 0, &err);
+    visit_start_struct(v, NULL, %(c_name)s, "%(name)s", 0, &err);
 ''',
-                     name=name)
+                     c_name=c_name, name=name)
         ret += gen_err_check()
-        ret += gen_visit_fields(arg_type.members, need_cast=True)
+        ret += gen_visit_fields(arg_type.members, need_cast=True,
+                                label='out_obj')
         ret += mcgen('''
+out_obj:
     visit_end_struct(v, &err);
     if (err) {
         goto out;
     }

     obj = qmp_output_get_qobject(qov);
-    g_assert(obj != NULL);
+    g_assert(obj);

     qdict_put_obj(qmp, "data", obj);
 ''')
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 45bc5a7..ed2a063 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1637,7 +1637,8 @@ def gen_err_check(label='out', skiperr=False):
                  label=label)


-def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
+def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False,
+                     label='out'):
     ret = ''
     if skiperr:
         errparg = 'NULL'
@@ -1665,7 +1666,7 @@ def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
                      c_type=memb.type.c_name(), prefix=prefix, cast=cast,
                      c_name=c_name(memb.name), name=memb.name,
                      errp=errparg)
-        ret += gen_err_check(skiperr=skiperr)
+        ret += gen_err_check(skiperr=skiperr, label=label)

         if memb.optional:
             pop_indent()
-- 
2.4.3

[Qemu-devel] [PATCH v6 16/23] qapi: Track all failures between visit_start/stop

[Eric Blake]

Inside the generated code between visit_start_struct() and
visit_end_struct(), we were blindly setting the error into
the caller's errp parameter.  But a future patch to split
visit_end_struct() will require that we take action based
on whether an error has occurred, which requires us to track
all actions through a local err.  Rewrite the visits to be
more in line with the other generated calls.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: based loosely on v5 7/46, but mostly a rewrite to get the last
generated code in the same form as all the others, so that the
later conversion to split visit_check_struct() will be easier
---
 scripts/qapi-visit.py | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 8c964b5..391bdfb 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -123,12 +123,18 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
     Error *err = NULL;

     visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
-    if (!err) {
-        if (*obj) {
-            visit_type_%(c_name)s_fields(v, obj, errp);
-        }
-        visit_end_struct(v, &err);
+    if (err) {
+        goto out;
     }
+    if (!*obj) {
+        goto out_obj;
+    }
+    visit_type_%(c_name)s_fields(v, obj, &err);
+out_obj:
+    error_propagate(errp, err);
+    err = NULL;
+    visit_end_struct(v, &err);
+out:
     error_propagate(errp, err);
 }
 ''',
-- 
2.4.3

[Qemu-devel] [PATCH v6 17/23] qapi: Eliminate empty visit_type_FOO_fields

[Eric Blake]

For empty structs, such as the 'Abort' helper type used as part
of the 'transaction' command, we were emitting a no-op
visit_type_FOO_fields().  Optimize things to instead omit calls
for empty structs.  Generated code changes resemble:

|-static void visit_type_Abort_fields(Visitor *v, Abort **obj, Error **errp)
|-{
|-    Error *err = NULL;
|-
|-    error_propagate(errp, err);
|-}
|-
| void visit_type_Abort(Visitor *v, Abort **obj, const char *name, Error **errp)
| {
|     Error *err = NULL;
|@@ -112,7 +105,6 @@ void visit_type_Abort(Visitor *v, Abort
|     if (!*obj) {
|         goto out_obj;
|     }
|-    visit_type_Abort_fields(v, obj, &err);
| out_obj:
|     error_propagate(errp, err);

Another reason for doing this optimization is that it gets us
closer to merging the code for visiting structs and unions:
since flat unions have no local members, they do not need to
have a visit_type_UNION_fields() emitted, even when they start
sharing the code used to visit structs.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: new patch
---
 scripts/qapi-visit.py | 53 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 24 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 391bdfb..ed4fb20 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -35,22 +35,22 @@ void visit_type_%(c_name)s(Visitor *v, %(c_type)sobj, const char *name, Error **


 def gen_visit_fields_decl(typ):
-    ret = ''
-    if typ.name not in struct_fields_seen:
-        ret += mcgen('''
+    if typ.is_empty() or typ.name in struct_fields_seen:
+        return ''
+
+    struct_fields_seen.add(typ.name)
+    return mcgen('''

 static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error **errp);
 ''',
-                     c_type=typ.c_name())
-        struct_fields_seen.add(typ.name)
-    return ret
+                 c_type=typ.c_name())


 def gen_visit_implicit_struct(typ):
-    if typ in implicit_structs_seen:
+    if typ.is_empty() or typ in implicit_structs_seen:
         return ''
+
     implicit_structs_seen.add(typ)
-
     ret = gen_visit_fields_decl(typ)

     ret += mcgen('''
@@ -74,7 +74,10 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error *
 def gen_visit_struct_fields(name, base, members):
     ret = ''

-    if base:
+    if (not base or base.is_empty()) and not members:
+        return ret
+
+    if base and not base.is_empty():
         ret += gen_visit_fields_decl(base)

     struct_fields_seen.add(name)
@@ -87,7 +90,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
 ''',
                  c_name=c_name(name))

-    if base:
+    if base and not base.is_empty():
         ret += mcgen('''
     visit_type_%(c_type)s_fields(v, (%(c_type)s **)obj, &err);
 ''',
@@ -96,13 +99,9 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e

     ret += gen_visit_fields(members, prefix='(*obj)->')

-    # 'goto out' produced for base, and by gen_visit_fields() for each member
-    if base or members:
-        ret += mcgen('''
+    ret += mcgen('''

 out:
-''')
-    ret += mcgen('''
     error_propagate(errp, err);
 }
 ''')
@@ -129,16 +128,22 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
     if (!*obj) {
         goto out_obj;
     }
-    visit_type_%(c_name)s_fields(v, obj, &err);
-out_obj:
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_struct(v, &err);
-out:
-    error_propagate(errp, err);
-}
 ''',
                  name=name, c_name=c_name(name))
+    if (base and not base.is_empty()) or members:
+        ret += mcgen('''
+    visit_type_%(c_name)s_fields(v, obj, &err);
+''',
+                     c_name=c_name(name))
+    ret += mcgen('''
+out_obj:
+    error_propagate(errp, err);
+    err = NULL;
+    visit_end_struct(v, &err);
+out:
+    error_propagate(errp, err);
+}
+''')

     return ret

@@ -300,7 +305,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
 ''',
                          c_type=simple_union_type.c_name(),
                          c_name=c_name(var.name))
-        else:
+        elif not var.type.is_empty():
             ret += mcgen('''
         visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
 ''',
-- 
2.4.3

[Qemu-devel] [PATCH v6 18/23] qapi: Canonicalize missing object to :empty

[Eric Blake]

Now that we elide unnecessary visits of empty types, we can
start using the special ':empty' type in more places.  By using
the empty type as the base class of every explicit struct or
union, and as the default data for any command or event, we can
simplify later logic in qapi-{visit,commands,event} by merely
checking whether the type is empty, without also having to worry
whether a type was even supplied.

Note that gen_object() in qapi-types still has to check for a
base, because it is also called for alternates (which have no
base).

No change to generated code.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: new patch
---
 scripts/qapi-commands.py                |  7 ++---
 scripts/qapi-event.py                   |  7 ++---
 scripts/qapi-types.py                   |  4 +--
 scripts/qapi-visit.py                   | 12 +++++----
 scripts/qapi.py                         | 22 ++++++++--------
 tests/qapi-schema/event-case.out        |  2 +-
 tests/qapi-schema/flat-union-empty.out  |  1 +
 tests/qapi-schema/ident-with-escape.out |  1 +
 tests/qapi-schema/indented-expr.out     |  4 +--
 tests/qapi-schema/qapi-schema-test.out  | 45 ++++++++++++++++++++++++++++++---
 tests/qapi-schema/union-clash-data.out  |  2 ++
 tests/qapi-schema/union-empty.out       |  1 +
 12 files changed, 78 insertions(+), 30 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 38cbffc..0f3cc57 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -65,7 +65,8 @@ def gen_marshal_vars(arg_type, ret_type):
 ''',
                      c_type=ret_type.c_type())

-    if arg_type and not arg_type.is_empty():
+    assert arg_type
+    if not arg_type.is_empty():
         ret += mcgen('''
     QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
     QapiDeallocVisitor *qdv;
@@ -97,7 +98,7 @@ def gen_marshal_vars(arg_type, ret_type):
 def gen_marshal_input_visit(arg_type, dealloc=False):
     ret = ''

-    if not arg_type or arg_type.is_empty():
+    if arg_type.is_empty():
         return ret

     if dealloc:
@@ -177,7 +178,7 @@ def gen_marshal(name, arg_type, ret_type):

     # 'goto out' produced by gen_marshal_input_visit->gen_visit_fields()
     # for each arg_type member, and by gen_call() for ret_type
-    if (arg_type and not arg_type.is_empty()) or ret_type:
+    if not arg_type.is_empty() or ret_type:
         ret += mcgen('''

 out:
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 5dc9726..7ee74a5 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -39,7 +39,8 @@ def gen_event_send(name, arg_type):
 ''',
                 proto=gen_event_send_proto(name, arg_type))

-    if arg_type and not arg_type.is_empty():
+    assert arg_type
+    if not arg_type.is_empty():
         ret += mcgen('''
     QObject *obj;
     QmpOutputVisitor *qov;
@@ -58,7 +59,7 @@ def gen_event_send(name, arg_type):
 ''',
                  name=name)

-    if arg_type and not arg_type.is_empty():
+    if not arg_type.is_empty():
         c_name = 'NULL'
         if not arg_type.is_implicit():
             c_name = '"%s"' % arg_type.c_name()
@@ -91,7 +92,7 @@ out_obj:
 ''',
                  c_enum=c_enum_const(event_enum_name, name))

-    if arg_type and not arg_type.is_empty():
+    if not arg_type.is_empty():
         ret += mcgen('''
 out:
     qmp_output_visitor_cleanup(qov);
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index af6b324..795a2bf 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -58,7 +58,7 @@ struct %(c_name)s {
 ''',
                 c_name=c_name(name))

-    if base:
+    if base and not base.is_empty():
         ret += mcgen('''
     /* Members inherited from %(c_name)s: */
 ''',
@@ -226,7 +226,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
     def visit_object_type(self, name, info, base, members, variants):
         self._fwdecl += gen_fwd_object_or_array(name)
         self.decl += gen_object(name, base, members, variants)
-        if base:
+        if not base.is_implicit():
             self.decl += gen_upcast(name, base)
         self._gen_type_cleanup(name)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index ed4fb20..421a5b5 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -74,10 +74,11 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error *
 def gen_visit_struct_fields(name, base, members):
     ret = ''

-    if (not base or base.is_empty()) and not members:
+    assert base
+    if base.is_empty() and not members:
         return ret

-    if base and not base.is_empty():
+    if not base.is_empty():
         ret += gen_visit_fields_decl(base)

     struct_fields_seen.add(name)
@@ -90,7 +91,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
 ''',
                  c_name=c_name(name))

-    if base and not base.is_empty():
+    if not base.is_empty():
         ret += mcgen('''
     visit_type_%(c_type)s_fields(v, (%(c_type)s **)obj, &err);
 ''',
@@ -246,7 +247,8 @@ out:
 def gen_visit_union(name, base, variants):
     ret = ''

-    if base:
+    assert base
+    if not base.is_empty():
         ret += gen_visit_fields_decl(base)

     for var in variants.variants:
@@ -270,7 +272,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
 ''',
                  c_name=c_name(name), name=name)

-    if base:
+    if not base.is_empty():
         ret += mcgen('''
     visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err);
 ''',
diff --git a/scripts/qapi.py b/scripts/qapi.py
index ed2a063..29eadb4 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -924,11 +924,11 @@ class QAPISchemaArrayType(QAPISchemaType):

 class QAPISchemaObjectType(QAPISchemaType):
     def __init__(self, name, info, base, local_members, variants):
-        # struct has local_members, optional base, and no variants
+        # struct has local_members, base, and no variants
         # flat union has base, variants, and no local_members
-        # simple union has local_members, variants, and no base
+        # simple union has local_members, variants, and base of :empty
         QAPISchemaType.__init__(self, name, info)
-        assert base is None or isinstance(base, str)
+        assert isinstance(base, str) or name == ':empty'
         for m in local_members:
             assert isinstance(m, QAPISchemaObjectTypeMember)
             m.set_owner(name)
@@ -1262,11 +1262,11 @@ class QAPISchema(object):

     def _make_implicit_object_type(self, name, info, role, members):
         if not members:
-            return None
+            return ':empty'
         # See also QAPISchemaObjectTypeMember._pretty_owner()
         name = ':obj-%s-%s' % (name, role)
         if not self.lookup_entity(name, QAPISchemaObjectType):
-            self._def_entity(QAPISchemaObjectType(name, info, None,
+            self._def_entity(QAPISchemaObjectType(name, info, ':empty',
                                                   members, None))
         return name

@@ -1293,7 +1293,7 @@ class QAPISchema(object):

     def _def_struct_type(self, expr, info):
         info['name'] = name = expr['struct']
-        base = expr.get('base')
+        base = expr.get('base', ':empty')
         data = expr['data']
         self._def_entity(QAPISchemaObjectType(name, info, base,
                                               self._make_members(data, info),
@@ -1313,7 +1313,7 @@ class QAPISchema(object):
     def _def_union_type(self, expr, info):
         info['name'] = name = expr['union']
         data = expr['data']
-        base = expr.get('base')
+        base = expr.get('base', ':empty')
         tag_name = expr.get('discriminator')
         tag_member = None
         if tag_name:
@@ -1347,11 +1347,11 @@ class QAPISchema(object):

     def _def_command(self, expr, info):
         info['name'] = name = expr['command']
-        data = expr.get('data')
+        data = expr.get('data', {})
         rets = expr.get('returns')
         gen = expr.get('gen', True)
         success_response = expr.get('success-response', True)
-        if isinstance(data, OrderedDict):
+        if isinstance(data, dict):
             data = self._make_implicit_object_type(
                 name, info, 'arg', self._make_members(data, info))
         if isinstance(rets, list):
@@ -1362,8 +1362,8 @@ class QAPISchema(object):

     def _def_event(self, expr, info):
         info['name'] = name = expr['event']
-        data = expr.get('data')
-        if isinstance(data, OrderedDict):
+        data = expr.get('data', {})
+        if isinstance(data, dict):
             data = self._make_implicit_object_type(
                 name, info, 'arg', self._make_members(data, info))
         self._def_entity(QAPISchemaEvent(name, info, data))
diff --git a/tests/qapi-schema/event-case.out b/tests/qapi-schema/event-case.out
index 6350d64..18866d9 100644
--- a/tests/qapi-schema/event-case.out
+++ b/tests/qapi-schema/event-case.out
@@ -1,4 +1,4 @@
 object :empty
 enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
     prefix QTYPE
-event oops None
+event oops :empty
diff --git a/tests/qapi-schema/flat-union-empty.out b/tests/qapi-schema/flat-union-empty.out
index eade2d5..1a58e07 100644
--- a/tests/qapi-schema/flat-union-empty.out
+++ b/tests/qapi-schema/flat-union-empty.out
@@ -1,5 +1,6 @@
 object :empty
 object Base
+    base :empty
     member type: Empty optional=False
 enum Empty []
 enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
diff --git a/tests/qapi-schema/ident-with-escape.out b/tests/qapi-schema/ident-with-escape.out
index 453e0b2..142cd19 100644
--- a/tests/qapi-schema/ident-with-escape.out
+++ b/tests/qapi-schema/ident-with-escape.out
@@ -1,5 +1,6 @@
 object :empty
 object :obj-fooA-arg
+    base :empty
     member bar1: str optional=False
 enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
     prefix QTYPE
diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out
index ce37ff5..5c25fcd 100644
--- a/tests/qapi-schema/indented-expr.out
+++ b/tests/qapi-schema/indented-expr.out
@@ -1,7 +1,7 @@
 object :empty
 enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
     prefix QTYPE
-command eins None -> None
+command eins :empty -> None
    gen=True success_response=True
-command zwei None -> None
+command zwei :empty -> None
    gen=True success_response=True
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index d8f9108..a2a3c8b 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -1,56 +1,78 @@
 object :empty
 object :obj-EVENT_C-arg
+    base :empty
     member a: int optional=True
     member b: UserDefOne optional=True
     member c: str optional=False
 object :obj-EVENT_D-arg
+    base :empty
     member a: EventStructOne optional=False
     member b: str optional=False
     member c: str optional=True
     member enum3: EnumOne optional=True
 object :obj-__org.qemu_x-command-arg
+    base :empty
     member a: __org.qemu_x-EnumList optional=False
     member b: __org.qemu_x-StructList optional=False
     member c: __org.qemu_x-Union2 optional=False
     member d: __org.qemu_x-Alt optional=False
 object :obj-anyList-wrapper
+    base :empty
     member data: anyList optional=False
 object :obj-boolList-wrapper
+    base :empty
     member data: boolList optional=False
 object :obj-guest-get-time-arg
+    base :empty
     member a: int optional=False
     member b: int optional=True
 object :obj-guest-sync-arg
+    base :empty
     member arg: any optional=False
 object :obj-int16List-wrapper
+    base :empty
     member data: int16List optional=False
 object :obj-int32List-wrapper
+    base :empty
     member data: int32List optional=False
 object :obj-int64List-wrapper
+    base :empty
     member data: int64List optional=False
 object :obj-int8List-wrapper
+    base :empty
     member data: int8List optional=False
 object :obj-intList-wrapper
+    base :empty
     member data: intList optional=False
 object :obj-numberList-wrapper
+    base :empty
     member data: numberList optional=False
 object :obj-sizeList-wrapper
+    base :empty
     member data: sizeList optional=False
 object :obj-str-wrapper
+    base :empty
     member data: str optional=False
 object :obj-strList-wrapper
+    base :empty
     member data: strList optional=False
 object :obj-uint16List-wrapper
+    base :empty
     member data: uint16List optional=False
 object :obj-uint32List-wrapper
+    base :empty
     member data: uint32List optional=False
 object :obj-uint64List-wrapper
+    base :empty
     member data: uint64List optional=False
 object :obj-uint8List-wrapper
+    base :empty
     member data: uint8List optional=False
 object :obj-user_def_cmd1-arg
+    base :empty
     member ud1a: UserDefOne optional=False
 object :obj-user_def_cmd2-arg
+    base :empty
     member ud1a: UserDefOne optional=False
     member ud1b: UserDefOne optional=True
 alternate AltIntNum
@@ -71,24 +93,28 @@ alternate AltStrInt
 alternate AltStrNum
     case s: str
     case n: number
-event EVENT_A None
-event EVENT_B None
+event EVENT_A :empty
+event EVENT_B :empty
 event EVENT_C :obj-EVENT_C-arg
 event EVENT_D :obj-EVENT_D-arg
 object Empty1
+    base :empty
 object Empty2
     base Empty1
 enum EnumOne ['value1', 'value2', 'value3']
 object EventStructOne
+    base :empty
     member struct1: UserDefOne optional=False
     member string: str optional=False
     member enum2: EnumOne optional=True
 object ForceArrays
+    base :empty
     member unused1: UserDefOneList optional=False
     member unused2: UserDefTwoList optional=False
     member unused3: TestStructList optional=False
 enum MyEnum []
 object NestedEnumsOne
+    base :empty
     member enum1: EnumOne optional=False
     member enum2: EnumOne optional=True
     member enum3: EnumOne optional=False
@@ -98,10 +124,12 @@ enum QEnumTwo ['value1', 'value2']
 enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
     prefix QTYPE
 object TestStruct
+    base :empty
     member integer: int optional=False
     member boolean: bool optional=False
     member string: str optional=False
 object UserDefA
+    base :empty
     member boolean: bool optional=False
     member a_b: int optional=True
 alternate UserDefAlternate
@@ -109,9 +137,11 @@ alternate UserDefAlternate
     case s: str
     case i: int
 object UserDefB
+    base :empty
     member intb: int optional=False
     member a-b: bool optional=True
 object UserDefC
+    base :empty
     member string1: str optional=False
     member string2: str optional=False
 object UserDefFlatUnion
@@ -127,6 +157,7 @@ object UserDefFlatUnion2
     case value2: UserDefB
     case value3: UserDefA
 object UserDefNativeListUnion
+    base :empty
     member type: UserDefNativeListUnionKind optional=False
     case integer: :obj-intList-wrapper
     case s8: :obj-int8List-wrapper
@@ -148,19 +179,23 @@ object UserDefOne
     member string: str optional=False
     member enum1: EnumOne optional=True
 object UserDefOptions
+    base :empty
     member i64: intList optional=True
     member u64: uint64List optional=True
     member u16: uint16List optional=True
     member i64x: int optional=True
     member u64x: uint64 optional=True
 object UserDefTwo
+    base :empty
     member string0: str optional=False
     member dict1: UserDefTwoDict optional=False
 object UserDefTwoDict
+    base :empty
     member string1: str optional=False
     member dict2: UserDefTwoDictDict optional=False
     member dict3: UserDefTwoDictDict optional=True
 object UserDefTwoDictDict
+    base :empty
     member userdef: UserDefOne optional=False
     member string: str optional=False
 object UserDefUnionBase
@@ -168,12 +203,14 @@ object UserDefUnionBase
     member string: str optional=False
     member enum1: EnumOne optional=False
 object UserDefZero
+    base :empty
     member integer: int optional=False
 event __ORG.QEMU_X-EVENT __org.qemu_x-Struct
 alternate __org.qemu_x-Alt
     case __org.qemu_x-branch: str
     case b: __org.qemu_x-Base
 object __org.qemu_x-Base
+    base :empty
     member __org.qemu_x-member1: __org.qemu_x-Enum optional=False
 enum __org.qemu_x-Enum ['__org.qemu_x-value']
 object __org.qemu_x-Struct
@@ -181,8 +218,10 @@ object __org.qemu_x-Struct
     member __org.qemu_x-member2: str optional=False
     member wchar-t: int optional=True
 object __org.qemu_x-Struct2
+    base :empty
     member array: __org.qemu_x-Union1List optional=False
 object __org.qemu_x-Union1
+    base :empty
     member type: __org.qemu_x-Union1Kind optional=False
     case __org.qemu_x-branch: :obj-str-wrapper
 enum __org.qemu_x-Union1Kind ['__org.qemu_x-branch']
@@ -196,7 +235,7 @@ command guest-get-time :obj-guest-get-time-arg -> int
    gen=True success_response=True
 command guest-sync :obj-guest-sync-arg -> any
    gen=True success_response=True
-command user_def_cmd None -> None
+command user_def_cmd :empty -> None
    gen=True success_response=True
 command user_def_cmd0 Empty2 -> Empty2
    gen=True success_response=True
diff --git a/tests/qapi-schema/union-clash-data.out b/tests/qapi-schema/union-clash-data.out
index f5752f4..19031a2 100644
--- a/tests/qapi-schema/union-clash-data.out
+++ b/tests/qapi-schema/union-clash-data.out
@@ -1,9 +1,11 @@
 object :empty
 object :obj-int-wrapper
+    base :empty
     member data: int optional=False
 enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
     prefix QTYPE
 object TestUnion
+    base :empty
     member type: TestUnionKind optional=False
     case data: :obj-int-wrapper
 enum TestUnionKind ['data']
diff --git a/tests/qapi-schema/union-empty.out b/tests/qapi-schema/union-empty.out
index bdf17e5..57fcbd1 100644
--- a/tests/qapi-schema/union-empty.out
+++ b/tests/qapi-schema/union-empty.out
@@ -2,5 +2,6 @@ object :empty
 enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
     prefix QTYPE
 object Union
+    base :empty
     member type: UnionKind optional=False
 enum UnionKind []
-- 
2.4.3

[Qemu-devel] [PATCH v6 19/23] qapi-visit: Unify struct and union visit

[Eric Blake]

We are finally at the point where gen_visit_struct() and
gen_visit_union() can be unified to a generic gen_visit_object().

The generated code for structs and for flat unions is unchanged.
For simple unions, a new visit_type_FOO_fields() is created,
wrapping the visit of the non-variant tag field:

|+static void visit_type_ChardevBackend_fields(Visitor *v, ChardevBackend **obj, Error **errp)
|+{
|+    Error *err = NULL;
|+
|+    visit_type_ChardevBackendKind(v, &(*obj)->type, "type", &err);
|+    if (err) {
|+        goto out;
|+    }
|+
|+out:
|+    error_propagate(errp, err);
|+}
|+
| void visit_type_ChardevBackend(Visitor *v, ChardevBackend **obj, const char *name, Error **errp)
| {
|     Error *err = NULL;
|@@ -2319,7 +2332,7 @@ void visit_type_ChardevBackend(Visitor *
|     if (!*obj) {
|         goto out_obj;
|     }
|-    visit_type_ChardevBackendKind(v, &(*obj)->type, "type", &err);
|+    visit_type_ChardevBackend_fields(v, obj, &err);
|     if (err) {

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: new patch
---
 scripts/qapi-visit.py | 133 +++++++++++++++++++-------------------------------
 1 file changed, 51 insertions(+), 82 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 421a5b5..f22ebeb 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -109,46 +109,6 @@ out:
     return ret


-def gen_visit_struct(name, base, members):
-    ret = gen_visit_struct_fields(name, base, members)
-
-    # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
-    # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
-    # rather than leaving it non-NULL. As currently written, the caller must
-    # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
-    ret += mcgen('''
-
-void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
-{
-    Error *err = NULL;
-
-    visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
-    if (err) {
-        goto out;
-    }
-    if (!*obj) {
-        goto out_obj;
-    }
-''',
-                 name=name, c_name=c_name(name))
-    if (base and not base.is_empty()) or members:
-        ret += mcgen('''
-    visit_type_%(c_name)s_fields(v, obj, &err);
-''',
-                     c_name=c_name(name))
-    ret += mcgen('''
-out_obj:
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_struct(v, &err);
-out:
-    error_propagate(errp, err);
-}
-''')
-
-    return ret
-
-
 def gen_visit_list(name, element_type):
     # FIXME: if *obj is NULL on entry, and the first visit_next_list()
     # assigns to *obj, while a later one fails, we should clean up *obj
@@ -244,18 +204,24 @@ out:
     return ret


-def gen_visit_union(name, base, variants):
+def gen_visit_object(name, base, members, variants):
     ret = ''

     assert base
     if not base.is_empty():
         ret += gen_visit_fields_decl(base)
+    if members:
+        ret += gen_visit_struct_fields(name, base, members)
+    if variants:
+        for var in variants.variants:
+            # Ugly special case for simple union TODO get rid of it
+            if not var.simple_union_type():
+                ret += gen_visit_implicit_struct(var.type)

-    for var in variants.variants:
-        # Ugly special case for simple union TODO get rid of it
-        if not var.simple_union_type():
-            ret += gen_visit_implicit_struct(var.type)
-
+    # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
+    # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
+    # rather than leaving it non-NULL. As currently written, the caller must
+    # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
     ret += mcgen('''

 void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
@@ -272,61 +238,71 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
 ''',
                  c_name=c_name(name), name=name)

-    if not base.is_empty():
+    if not base.is_empty() or members:
+        if members:
+            type_name = c_name(name)
+            cast = ''
+        else:
+            type_name = base.c_name()
+            cast = '(%s **)' % type_name
         ret += mcgen('''
-    visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err);
+    visit_type_%(c_name)s_fields(v, %(cast)sobj, &err);
 ''',
-                     c_name=base.c_name())
-    else:
+                     c_name=type_name, cast=cast)
+        if variants:
+            ret += gen_err_check(label='out_obj')
+
+    if variants:
         ret += mcgen('''
-    visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
-''',
-                     c_type=variants.tag_member.type.c_name(),
-                     c_name=c_name(variants.tag_member.name),
-                     name=variants.tag_member.name)
-    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))
+                     c_name=c_name(variants.tag_member.name))

-    for var in variants.variants:
-        # TODO ugly special case for simple union
-        simple_union_type = var.simple_union_type()
-        ret += mcgen('''
+        for var in variants.variants:
+            # TODO ugly special case for simple union
+            simple_union_type = var.simple_union_type()
+            ret += mcgen('''
     case %(case)s:
 ''',
-                     case=c_enum_const(variants.tag_member.type.name,
-                                       var.name))
-        if simple_union_type:
-            ret += mcgen('''
+                         case=c_enum_const(variants.tag_member.type.name,
+                                           var.name))
+            if simple_union_type:
+                ret += mcgen('''
         visit_type_%(c_type)s(v, &(*obj)->u.%(c_name)s, "data", &err);
 ''',
-                         c_type=simple_union_type.c_name(),
-                         c_name=c_name(var.name))
-        elif not var.type.is_empty():
-            ret += mcgen('''
+                             c_type=simple_union_type.c_name(),
+                             c_name=c_name(var.name))
+            elif not var.type.is_empty():
+                ret += mcgen('''
         visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
 ''',
-                         c_type=var.type.c_name(),
-                         c_name=c_name(var.name))
-        ret += mcgen('''
+                             c_type=var.type.c_name(),
+                             c_name=c_name(var.name))
+            ret += mcgen('''
         break;
 ''')

-    ret += mcgen('''
+        ret += mcgen('''
     default:
         abort();
     }
+''')
+
+    ret += mcgen('''
 out_obj:
+''')
+    if variants:
+        ret += mcgen('''
     error_propagate(errp, err);
     err = NULL;
     if (*obj) {
         visit_end_union(v, !!(*obj)->u.data, &err);
     }
+''')
+    ret += mcgen('''
     error_propagate(errp, err);
     err = NULL;
     visit_end_struct(v, &err);
@@ -387,14 +363,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):

     def visit_object_type(self, name, info, base, members, variants):
         self.decl += gen_visit_decl(name)
-        if variants:
-            if members:
-                # Members other than variants.tag_member not implemented
-                assert len(members) == 1
-                assert members[0] == variants.tag_member
-            self.defn += gen_visit_union(name, base, variants)
-        else:
-            self.defn += gen_visit_struct(name, base, members)
+        self.defn += gen_visit_object(name, base, members, variants)

     def visit_alternate_type(self, name, info, variants):
         self.decl += gen_visit_decl(name)
-- 
2.4.3

[Qemu-devel] [PATCH v6 20/23] qapi: Rework deallocation of partial struct

[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 the NULL pointer. But the fix it proposed
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), so it feels fairly dirty.  A better fix is to tweak all
of the generated visit_type_implicit_FOO() functions to avoid
dereferencing NULL in the first place, by not visiting the
fields if the struct pointer itself is not present, at which
point we no longer even need visit_start_union().  And no one
was implementing visit_end_union() callbacks.

While rewriting the code, use patterns that are closer to what
is used elsewhere in the generated visitors, by using 'goto'
to cleanup labels rather than putting followup code under 'if'
conditions.  The change keeps the contract that any successful
use of visit_start_implicit_struct() will be paired with a
matching visit_end_implicit_struct(), even if intermediate
processing is skipped.

As an example of the changes to generated code:
|@@ -1331,10 +1331,16 @@ static void visit_type_implicit_Blockdev
|     Error *err = NULL;
|
|     visit_start_implicit_struct(v, (void **)obj, sizeof(BlockdevOptionsArchipelago), &err);
|-    if (!err) {
|-        visit_type_BlockdevOptionsArchipelago_fields(v, obj, errp);
|-        visit_end_implicit_struct(v);
|+    if (err) {
|+        goto out;
|+    }
|+    if (obj && !*obj) {
|+        goto out_obj;
|     }
|+    visit_type_BlockdevOptionsArchipelago_fields(v, obj, &err);
|+out_obj:
|+    visit_end_implicit_struct(v);
|+out:
|     error_propagate(errp, err);
| }
...
|@@ -1479,9 +1539,6 @@ void visit_type_BlockdevOptions(Visitor
|     if (err) {
|         goto out_obj;
|     }
|-    if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
|-        goto out_obj;
|-    }
|     switch ((*obj)->driver) {
|     case BLOCKDEV_DRIVER_ARCHIPELAGO:
|         visit_type_implicit_BlockdevOptionsArchipelago(v, &(*obj)->u.archipelago, &err);
|@@ -1570,11 +1627,6 @@ void visit_type_BlockdevOptions(Visitor
| out_obj:
|     error_propagate(errp, err);
|     err = NULL;
|-    if (*obj) {
|-        visit_end_union(v, !!(*obj)->u.data, &err);
|-    }
|-    error_propagate(errp, err);
|-    err = NULL;
|     visit_end_struct(v, &err);

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: rebase due to deferring 7/46, and gen_err_check() improvements;
rewrite gen_visit_implicit_struct() more like other patterns
---
 include/qapi/visitor-impl.h |  5 -----
 include/qapi/visitor.h      | 12 ------------
 qapi/qapi-dealloc-visitor.c | 26 --------------------------
 qapi/qapi-visit-core.c      | 15 ---------------
 scripts/qapi-visit.py       | 25 +++++++++----------------
 5 files changed, 9 insertions(+), 74 deletions(-)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 36984a7..018f419 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -78,11 +78,6 @@ struct Visitor

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

 /**
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index b4ed469..cc1ff6d 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -236,16 +236,4 @@ void visit_type_number(Visitor *v, double *obj, const char *name,
  */
 void visit_type_any(Visitor *v, QObject **obj, const char *name, Error **errp);

-/**
- * Mark the start of visiting the branches of a union. Return true if
- * @data_present.
- * FIXME: Should not be needed
- */
-bool visit_start_union(Visitor *v, bool data_present, Error **errp);
-/**
- * Mark the end of union branches, after visit_start_union().
- * FIXME: Should not be needed
- */
-void visit_end_union(Visitor *v, bool data_present, Error **errp);
-
 #endif
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 8246070..8b489a4 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -171,31 +171,6 @@ static void qapi_dealloc_type_enum(Visitor *v, 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;
@@ -226,7 +201,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);

diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 477d73a..a193a04 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -58,21 +58,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;
-}
-
-void visit_end_union(Visitor *v, bool data_present, Error **errp)
-{
-    if (v->end_union) {
-        v->end_union(v, data_present, errp);
-    }
-}
-
 bool visit_optional(Visitor *v, bool *present, const char *name)
 {
     if (v->optional) {
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index f22ebeb..43233d6 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -60,10 +60,16 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error *
     Error *err = NULL;

     visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err);
-    if (!err) {
-        visit_type_%(c_type)s_fields(v, obj, errp);
-        visit_end_implicit_struct(v);
+    if (err) {
+        goto out;
     }
+    if (obj && !*obj) {
+        goto out_obj;
+    }
+    visit_type_%(c_type)s_fields(v, obj, &err);
+out_obj:
+    visit_end_implicit_struct(v);
+out:
     error_propagate(errp, err);
 }
 ''',
@@ -254,9 +260,6 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error

     if variants:
         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))
@@ -293,16 +296,6 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error

     ret += mcgen('''
 out_obj:
-''')
-    if variants:
-        ret += mcgen('''
-    error_propagate(errp, err);
-    err = NULL;
-    if (*obj) {
-        visit_end_union(v, !!(*obj)->u.data, &err);
-    }
-''')
-    ret += mcgen('''
     error_propagate(errp, err);
     err = NULL;
     visit_end_struct(v, &err);
-- 
2.4.3

[Qemu-devel] [PATCH v6 21/23] qapi: Simplify extra member error reporting in input visitors

[Eric Blake]

When reporting that an unvisited member remains at the end of an
input visit for a struct, we were using g_hash_table_find()
coupled with a callback function that always returns true, to
locate an arbitrary member of the hash table.  But if all we
need is one entry, we can get that from a single iteration on an
iterator, without needing to split logic to a callback function.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: new patch, based on comments on RFC against v5 7/46
---
 qapi/opts-visitor.c      | 12 +++---------
 qapi/qmp-input-visitor.c | 14 +++++---------
 2 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 37f172d..46dd9fe 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -150,17 +150,11 @@ opts_start_struct(Visitor *v, void **obj, const char *kind,
 }


-static gboolean
-ghr_true(gpointer ign_key, gpointer ign_value, gpointer ign_user_data)
-{
-    return TRUE;
-}
-
-
 static void
 opts_end_struct(Visitor *v, Error **errp)
 {
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+    GHashTableIter iter;
     GQueue *any;

     if (--ov->depth > 0) {
@@ -168,8 +162,8 @@ opts_end_struct(Visitor *v, Error **errp)
     }

     /* we should have processed all (distinct) QemuOpt instances */
-    any = g_hash_table_find(ov->unprocessed_opts, &ghr_true, NULL);
-    if (any) {
+    g_hash_table_iter_init(&iter, ov->unprocessed_opts);
+    if (g_hash_table_iter_next(&iter, NULL, (void **)&any)) {
         const QemuOpt *first;

         first = g_queue_peek_head(any);
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 9ff1e75..9dbe025 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -88,12 +88,6 @@ static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp)
     qiv->nb_stack++;
 }

-/** Only for qmp_input_pop. */
-static gboolean always_true(gpointer key, gpointer val, gpointer user_pkey)
-{
-    *(const char **)user_pkey = (const char *)key;
-    return TRUE;
-}

 static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
 {
@@ -102,9 +96,11 @@ static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
     if (qiv->strict) {
         GHashTable * const top_ht = qiv->stack[qiv->nb_stack - 1].h;
         if (top_ht) {
-            if (g_hash_table_size(top_ht)) {
-                const char *key;
-                g_hash_table_find(top_ht, always_true, &key);
+            GHashTableIter iter;
+            const char *key;
+
+            g_hash_table_iter_init(&iter, top_ht);
+            if (g_hash_table_iter_next(&iter, (void **)&key, NULL)) {
                 error_setg(errp, QERR_QMP_EXTRA_MEMBER, key);
             }
             g_hash_table_unref(top_ht);
-- 
2.4.3

[Qemu-devel] [PATCH v6 22/23] qapi: Split visit_end_struct() into pieces

[Eric Blake]

As mentioned in previous patches, we want to call visit_end_struct()
functions unconditionally, so that visitors can release resources
tied up since the matching visit_start_struct() without also having
to worry about error priority if more than one error occurs.

Even though error_propagate() can be safely used to ignore a second
error during cleanup caused by a first error, it is simpler if the
cleanup cannot set an error, and we instead split the task of
checking that an input visitor has no unvisited input as a new
function visit_check_struct(), called only if all prior steps are
successful.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: new patch, revised version of RFC based on discussion of v5 7/46
---
 hmp.c                       |  8 +++-----
 hw/ppc/spapr_drc.c          |  3 ++-
 hw/virtio/virtio-balloon.c  | 12 ++++++------
 include/qapi/visitor-impl.h |  4 +++-
 include/qapi/visitor.h      | 12 ++++++++++--
 qapi/opts-visitor.c         | 17 +++++++++++++++--
 qapi/qapi-dealloc-visitor.c |  2 +-
 qapi/qapi-visit-core.c      | 11 +++++++++--
 qapi/qmp-input-visitor.c    | 34 +++++++++++++++++++---------------
 qapi/qmp-output-visitor.c   |  2 +-
 qom/object.c                |  5 ++---
 scripts/qapi-event.py       |  3 ++-
 scripts/qapi-visit.py       |  9 ++++-----
 vl.c                        |  9 ++++-----
 14 files changed, 81 insertions(+), 50 deletions(-)

diff --git a/hmp.c b/hmp.c
index ec1d682..b09f78b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1663,7 +1663,6 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
 void hmp_object_add(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
-    Error *err_end = NULL;
     QemuOpts *opts;
     char *type = NULL;
     char *id = NULL;
@@ -1696,13 +1695,12 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
     if (err) {
         goto out_end;
     }
-
+    visit_check_struct(v, &err);
 out_end:
-    visit_end_struct(v, &err_end);
-    if (!err && !err_end) {
+    visit_end_struct(v);
+    if (!err) {
         object_add(type, id, pdict, v, &err);
     }
-    error_propagate(&err, err_end);
 out_clean:
     opts_visitor_cleanup(ov);

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 03a1879..5ff270f 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -281,7 +281,8 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque,
         case FDT_END_NODE:
             /* shouldn't ever see an FDT_END_NODE before FDT_BEGIN_NODE */
             g_assert(fdt_depth > 0);
-            visit_end_struct(v, &error_abort);
+            visit_check_struct(v, &error_abort);
+            visit_end_struct(v);
             fdt_depth--;
             break;
         case FDT_PROP: {
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 1ce987a..531913b 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -136,15 +136,15 @@ static void balloon_stats_get_all(Object *obj, struct Visitor *v,
             goto out_nested;
         }
     }
+    visit_check_struct(v, &err);
 out_nested:
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_struct(v, &err);
+    visit_end_struct(v);

+    if (!err) {
+        visit_check_struct(v, &err);
+    }
 out_end:
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_struct(v, &err);
+    visit_end_struct(v);
 out:
     error_propagate(errp, err);
 }
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 018f419..5350bdf 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -28,8 +28,10 @@ struct Visitor
      * currently visit structs). */
     void (*start_struct)(Visitor *v, void **obj, const char *kind,
                          const char *name, size_t size, Error **errp);
+    /* May be NULL; most useful for input visitors. */
+    void (*check_struct)(Visitor *v, Error **errp);
     /* Must be provided if start_struct is present. */
-    void (*end_struct)(Visitor *v, Error **errp);
+    void (*end_struct)(Visitor *v);

     /* May be NULL; most useful for input visitors. */
     void (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index cc1ff6d..f9ea325 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -57,11 +57,19 @@ typedef struct GenericList
 void visit_start_struct(Visitor *v, void **obj, const char *kind,
                         const char *name, size_t size, Error **errp);
 /**
+ * Prepare for completing a struct visit.
+ * Should be called prior to visit_end_struct() if all other intermediate
+ * visit steps were successful, to allow the caller one last chance to
+ * report errors such as remaining data that was not consumed by the visit.
+ */
+void visit_check_struct(Visitor *v, Error **errp);
+/**
  * Complete a struct visit started earlier.
  * Must be called after any successful use of visit_start_struct(),
- * even if intermediate processing was skipped due to errors.
+ * even if intermediate processing was skipped due to errors, to allow
+ * the backend to release any resources.
  */
-void visit_end_struct(Visitor *v, Error **errp);
+void visit_end_struct(Visitor *v);

 /**
  * Prepare to visit an implicit struct.
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 46dd9fe..db5fc1a 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -151,13 +151,13 @@ opts_start_struct(Visitor *v, void **obj, const char *kind,


 static void
-opts_end_struct(Visitor *v, Error **errp)
+opts_check_struct(Visitor *v, Error **errp)
 {
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
     GHashTableIter iter;
     GQueue *any;

-    if (--ov->depth > 0) {
+    if (ov->depth > 0) {
         return;
     }

@@ -169,6 +169,18 @@ opts_end_struct(Visitor *v, Error **errp)
         first = g_queue_peek_head(any);
         error_setg(errp, QERR_INVALID_PARAMETER, first->name);
     }
+}
+
+
+static void
+opts_end_struct(Visitor *v)
+{
+    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+
+    if (--ov->depth > 0) {
+        return;
+    }
+
     g_hash_table_destroy(ov->unprocessed_opts);
     ov->unprocessed_opts = NULL;
     if (ov->fake_id_opt) {
@@ -500,6 +512,7 @@ opts_visitor_new(const QemuOpts *opts)
     ov = g_malloc0(sizeof *ov);

     ov->visitor.start_struct = &opts_start_struct;
+    ov->visitor.check_struct = &opts_check_struct;
     ov->visitor.end_struct   = &opts_end_struct;

     ov->visitor.start_list = &opts_start_list;
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 8b489a4..14dc7c3 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -67,7 +67,7 @@ static void qapi_dealloc_start_struct(Visitor *v, void **obj, const char *kind,
     qapi_dealloc_push(qov, obj);
 }

-static void qapi_dealloc_end_struct(Visitor *v, Error **errp)
+static void qapi_dealloc_end_struct(Visitor *v)
 {
     QapiDeallocVisitor *qov = to_qov(v);
     void **obj = qapi_dealloc_pop(qov);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index a193a04..00f446e 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -23,9 +23,16 @@ void visit_start_struct(Visitor *v, void **obj, const char *kind,
     v->start_struct(v, obj, kind, name, size, errp);
 }

-void visit_end_struct(Visitor *v, Error **errp)
+void visit_check_struct(Visitor *v, Error **errp)
 {
-    v->end_struct(v, errp);
+    if (v->check_struct) {
+        v->check_struct(v, errp);
+    }
+}
+
+void visit_end_struct(Visitor *v)
+{
+    v->end_struct(v);
 }

 void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 9dbe025..0270d25 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -89,8 +89,10 @@ static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp)
 }


-static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
+static void qmp_input_check_struct(Visitor *v, Error **errp)
 {
+    QmpInputVisitor *qiv = to_qiv(v);
+
     assert(qiv->nb_stack > 0);

     if (qiv->strict) {
@@ -103,6 +105,19 @@ static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
             if (g_hash_table_iter_next(&iter, (void **)&key, NULL)) {
                 error_setg(errp, QERR_QMP_EXTRA_MEMBER, key);
             }
+        }
+    }
+}
+
+static void qmp_input_pop(Visitor *v)
+{
+    QmpInputVisitor *qiv = to_qiv(v);
+
+    assert(qiv->nb_stack > 0);
+
+    if (qiv->strict) {
+        GHashTable * const top_ht = qiv->stack[qiv->nb_stack - 1].h;
+        if (top_ht) {
             g_hash_table_unref(top_ht);
         }
     }
@@ -134,12 +149,6 @@ static void qmp_input_start_struct(Visitor *v, void **obj, const char *kind,
     }
 }

-static void qmp_input_end_struct(Visitor *v, Error **errp)
-{
-    QmpInputVisitor *qiv = to_qiv(v);
-
-    qmp_input_pop(qiv, errp);
-}

 static void qmp_input_start_implicit_struct(Visitor *v, void **obj,
                                             size_t size, Error **errp)
@@ -193,12 +202,6 @@ static GenericList *qmp_input_next_list(Visitor *v, GenericList **list,
     return entry;
 }

-static void qmp_input_end_list(Visitor *v)
-{
-    QmpInputVisitor *qiv = to_qiv(v);
-
-    qmp_input_pop(qiv, &error_abort);
-}

 static void qmp_input_get_next_type(Visitor *v, QType *type, bool promote_int,
                                     const char *name, Error **errp)
@@ -341,11 +344,12 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
     v = g_malloc0(sizeof(*v));

     v->visitor.start_struct = qmp_input_start_struct;
-    v->visitor.end_struct = qmp_input_end_struct;
+    v->visitor.check_struct = qmp_input_check_struct;
+    v->visitor.end_struct = qmp_input_pop;
     v->visitor.start_implicit_struct = qmp_input_start_implicit_struct;
     v->visitor.start_list = qmp_input_start_list;
     v->visitor.next_list = qmp_input_next_list;
-    v->visitor.end_list = qmp_input_end_list;
+    v->visitor.end_list = qmp_input_pop;
     v->visitor.type_enum = input_type_enum;
     v->visitor.type_int64 = qmp_input_type_int64;
     v->visitor.type_uint64 = qmp_input_type_uint64;
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index 8035649..af1e01b 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -122,7 +122,7 @@ static void qmp_output_start_struct(Visitor *v, void **obj, const char *kind,
     qmp_output_push(qov, dict);
 }

-static void qmp_output_end_struct(Visitor *v, Error **errp)
+static void qmp_output_end_struct(Visitor *v)
 {
     QmpOutputVisitor *qov = to_qov(v);
     qmp_output_pop(qov);
diff --git a/qom/object.c b/qom/object.c
index d751569..3d5c9d7 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1904,10 +1904,9 @@ static void property_get_tm(Object *obj, Visitor *v, void *opaque,
     if (err) {
         goto out_end;
     }
+    visit_check_struct(v, &err);
 out_end:
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_struct(v, errp);
+    visit_end_struct(v);
 out:
     error_propagate(errp, err);

diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 7ee74a5..68d59e1 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -74,8 +74,9 @@ def gen_event_send(name, arg_type):
         ret += gen_visit_fields(arg_type.members, need_cast=True,
                                 label='out_obj')
         ret += mcgen('''
+    visit_check_struct(v, &err);
 out_obj:
-    visit_end_struct(v, &err);
+    visit_end_struct(v);
     if (err) {
         goto out;
     }
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 43233d6..c28208e 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -255,8 +255,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
     visit_type_%(c_name)s_fields(v, %(cast)sobj, &err);
 ''',
                      c_name=type_name, cast=cast)
-        if variants:
-            ret += gen_err_check(label='out_obj')
+        ret += gen_err_check(label='out_obj')

     if variants:
         ret += mcgen('''
@@ -293,12 +292,12 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
         abort();
     }
 ''')
+        ret += gen_err_check(label='out_obj')

     ret += mcgen('''
+    visit_check_struct(v, &err);
 out_obj:
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_struct(v, &err);
+    visit_end_struct(v);
 out:
     error_propagate(errp, err);
 }
diff --git a/vl.c b/vl.c
index 4e69815..32e4300 100644
--- a/vl.c
+++ b/vl.c
@@ -2828,7 +2828,6 @@ static bool object_create_delayed(const char *type)
 static int object_create(void *opaque, QemuOpts *opts, Error **errp)
 {
     Error *err = NULL;
-    Error *err_end = NULL;
     char *type = NULL;
     char *id = NULL;
     OptsVisitor *ov;
@@ -2848,7 +2847,7 @@ static int object_create(void *opaque, QemuOpts *opts, Error **errp)
     qdict_del(pdict, "qom-type");
     visit_type_str(v, &type, "qom-type", &err);
     if (err) {
-        goto out;
+        goto out_end;
     }
     if (!type_predicate(type)) {
         goto out_end;
@@ -2859,10 +2858,10 @@ static int object_create(void *opaque, QemuOpts *opts, Error **errp)
     if (err) {
         goto out_end;
     }
-
+    visit_check_struct(v, &err);
 out_end:
-    visit_end_struct(v, &err_end);
-    if (!err && !err_end) {
+    visit_end_struct(v);
+    if (!err) {
         object_add(type, id, pdict, v, &err);
     }

-- 
2.4.3

[Qemu-devel] [PATCH v6 23/23] qapi: Change visit_type_FOO() to no longer return partial objects

[Eric Blake]

Returning a partial object on error is an invitation for a careless
caller to leak memory.  As no one outside the testsuite was actually
relying on these semantics, it is cleaner to just document and
guarantee that ALL visit_type_FOO() functions leave a safe value
in *obj when an error is encountered during an input visitor.

Since input visitors have blind assignment semantics, we have to
track the result of whether an assignment is made all the way down
to each visitor callback implementation.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: rebase on top of earlier doc and formatting improvements, mention
that *obj can be uninitialized on entry to an input visitor, rework
semantics to keep valgrind happy on uninitialized input, break some
long lines
---
 include/qapi/visitor-impl.h    |  6 ++---
 include/qapi/visitor.h         | 52 +++++++++++++++++++++++++++++++-----------
 qapi/opts-visitor.c            |  8 ++++---
 qapi/qapi-dealloc-visitor.c    |  9 +++++---
 qapi/qapi-visit-core.c         | 13 ++++++-----
 qapi/qmp-input-visitor.c       | 17 +++++++++-----
 qapi/qmp-output-visitor.c      |  6 +++--
 qapi/string-input-visitor.c    |  3 ++-
 qapi/string-output-visitor.c   |  3 ++-
 scripts/qapi-visit.py          | 40 ++++++++++++++++++++++++--------
 tests/test-qmp-commands.c      | 13 +++++------
 tests/test-qmp-input-strict.c  | 19 +++++++--------
 tests/test-qmp-input-visitor.c | 10 ++------
 13 files changed, 125 insertions(+), 74 deletions(-)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 5350bdf..a51aa2a 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -26,7 +26,7 @@ struct Visitor
 {
     /* Must be provided to visit structs (the string visitors do not
      * currently visit structs). */
-    void (*start_struct)(Visitor *v, void **obj, const char *kind,
+    bool (*start_struct)(Visitor *v, void **obj, const char *kind,
                          const char *name, size_t size, Error **errp);
     /* May be NULL; most useful for input visitors. */
     void (*check_struct)(Visitor *v, Error **errp);
@@ -34,13 +34,13 @@ struct Visitor
     void (*end_struct)(Visitor *v);

     /* May be NULL; most useful for input visitors. */
-    void (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
+    bool (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
                                   Error **errp);
     /* May be NULL */
     void (*end_implicit_struct)(Visitor *v);

     /* Must be set */
-    void (*start_list)(Visitor *v, const char *name, Error **errp);
+    bool (*start_list)(Visitor *v, const char *name, Error **errp);
     /* Must be set */
     GenericList *(*next_list)(Visitor *v, GenericList **list, Error **errp);
     /* Must be set */
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index f9ea325..0b63a7a 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -30,6 +30,27 @@
  * visitor-impl.h.
  */

+/* All qapi types have a corresponding function with a signature
+ * roughly compatible with this:
+ *
+ * void visit_type_FOO(Visitor *v, void *obj, const char *name, Error **errp);
+ *
+ * where *@obj is itself a pointer or a scalar.  The visit functions for
+ * built-in types are declared here, while the functions for qapi-defined
+ * struct, union, enum, and list types are generated (see qapi-visit.h).
+ * Input visitors may receive an uninitialized *@obj, and guarantee a
+ * safe value is assigned (a new object on success, or NULL on failure).
+ * Output visitors expect *@obj to be properly initialized on entry.
+ *
+ * Additionally, all qapi structs have a generated function compatible
+ * with this:
+ *
+ * void qapi_free_FOO(void *obj);
+ *
+ * which behaves like free(), even if @obj is NULL or was only partially
+ * allocated before encountering an error.
+ */
+
 /* This struct is layout-compatible with all other *List structs
  * created by the qapi generator. */
 typedef struct GenericList
@@ -51,10 +72,12 @@ typedef struct GenericList
  * bytes, without regards to the previous value of *@obj. For other
  * visitors, *@obj is the object to visit. Set *@errp on failure.
  *
- * FIXME: *@obj can be modified even on error; this can lead to
- * memory leaks if clients aren't careful.
+ * Returns true if *@obj was allocated; if that happens, and an error
+ * occurs any time before the matching visit_end_struct(), then the
+ * caller (usually a visit_type_FOO() function) knows to undo the
+ * allocation before returning control further.
  */
-void visit_start_struct(Visitor *v, void **obj, const char *kind,
+bool visit_start_struct(Visitor *v, void **obj, const char *kind,
                         const char *name, size_t size, Error **errp);
 /**
  * Prepare for completing a struct visit.
@@ -73,14 +96,12 @@ void visit_end_struct(Visitor *v);

 /**
  * Prepare to visit an implicit struct.
- * Similar to visit_start_struct(), except that in implicit struct
- * represents a subset of keys that are present at the same nesting level
- * of a common object, rather than a new object at a deeper nesting level.
- *
- * FIXME: *@obj can be modified even on error; this can lead to
- * memory leaks if clients aren't careful.
+ * Similar to visit_start_struct(), including return semantics, except
+ * that an implicit struct represents a subset of keys that are present
+ * at the same nesting level of a common object, rather than a new object
+ * at a deeper nesting level.
  */
-void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
+bool visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
                                  Error **errp);
 /**
  * Complete an implicit struct visit started earlier.
@@ -96,9 +117,12 @@ void visit_end_implicit_struct(Visitor *v);
  * @name will be NULL if this is visited as part of another list.
  * After calling this, the elements must be collected until
  * visit_next_list() returns NULL, then visit_end_list() must be
- * used to complete the visit.
+ * used to complete the visit.  Return true if the visit may
+ * result in allocation, such that the caller (usually a visit_type_FOO()
+ * function) knows to undo any allocation if an error occurs before
+ * the matching visit_end_list().
  */
-void visit_start_list(Visitor *v, const char *name, Error **errp);
+bool visit_start_list(Visitor *v, const char *name, Error **errp);
 /**
  * Collect the next list member and append it to *@list.
  * Start with *@list of NULL, then subsequent iterations should pass
@@ -106,7 +130,9 @@ void visit_start_list(Visitor *v, const char *name, Error **errp);
  * loop until a NULL return or error occurs; for each non-NULL return,
  * the caller must then call the appropriate visit_type_*() for the
  * element type of the list, with that function's name parameter set
- * to NULL.
+ * to NULL.  If an error occurs, then the caller (usually a
+ * visit_type_FOO() function) knows to undo the list allocation before
+ * returning control further.
  */
 GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
 /**
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index db5fc1a..33c1868 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -117,7 +117,7 @@ opts_visitor_insert(GHashTable *unprocessed_opts, const QemuOpt *opt)
 }


-static void
+static bool
 opts_start_struct(Visitor *v, void **obj, const char *kind,
                   const char *name, size_t size, Error **errp)
 {
@@ -128,7 +128,7 @@ opts_start_struct(Visitor *v, void **obj, const char *kind,
         *obj = g_malloc0(size > 0 ? size : 1);
     }
     if (ov->depth++ > 0) {
-        return;
+        return true;
     }

     ov->unprocessed_opts = g_hash_table_new_full(&g_str_hash, &g_str_equal,
@@ -147,6 +147,7 @@ opts_start_struct(Visitor *v, void **obj, const char *kind,
         ov->fake_id_opt->str = g_strdup(ov->opts_root->id);
         opts_visitor_insert(ov->unprocessed_opts, ov->fake_id_opt);
     }
+    return true;
 }


@@ -205,7 +206,7 @@ lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp)
 }


-static void
+static bool
 opts_start_list(Visitor *v, const char *name, Error **errp)
 {
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
@@ -216,6 +217,7 @@ opts_start_list(Visitor *v, const char *name, Error **errp)
     if (ov->repeated_opts != NULL) {
         ov->list_mode = LM_STARTED;
     }
+    return true;
 }


diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 14dc7c3..a7dc43f 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -59,12 +59,13 @@ static void *qapi_dealloc_pop(QapiDeallocVisitor *qov)
     return value;
 }

-static void qapi_dealloc_start_struct(Visitor *v, void **obj, const char *kind,
+static bool qapi_dealloc_start_struct(Visitor *v, void **obj, const char *kind,
                                       const char *name, size_t unused,
                                       Error **errp)
 {
     QapiDeallocVisitor *qov = to_qov(v);
     qapi_dealloc_push(qov, obj);
+    return false;
 }

 static void qapi_dealloc_end_struct(Visitor *v)
@@ -76,13 +77,14 @@ static void qapi_dealloc_end_struct(Visitor *v)
     }
 }

-static void qapi_dealloc_start_implicit_struct(Visitor *v,
+static bool qapi_dealloc_start_implicit_struct(Visitor *v,
                                                void **obj,
                                                size_t size,
                                                Error **errp)
 {
     QapiDeallocVisitor *qov = to_qov(v);
     qapi_dealloc_push(qov, obj);
+    return false;
 }

 static void qapi_dealloc_end_implicit_struct(Visitor *v)
@@ -94,10 +96,11 @@ static void qapi_dealloc_end_implicit_struct(Visitor *v)
     }
 }

-static void qapi_dealloc_start_list(Visitor *v, const char *name, Error **errp)
+static bool qapi_dealloc_start_list(Visitor *v, const char *name, Error **errp)
 {
     QapiDeallocVisitor *qov = to_qov(v);
     qapi_dealloc_push(qov, NULL);
+    return false;
 }

 static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList **listp,
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 00f446e..c1abe1e 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -17,10 +17,10 @@
 #include "qapi/visitor.h"
 #include "qapi/visitor-impl.h"

-void visit_start_struct(Visitor *v, void **obj, const char *kind,
+bool visit_start_struct(Visitor *v, void **obj, const char *kind,
                         const char *name, size_t size, Error **errp)
 {
-    v->start_struct(v, obj, kind, name, size, errp);
+    return v->start_struct(v, obj, kind, name, size, errp);
 }

 void visit_check_struct(Visitor *v, Error **errp)
@@ -35,12 +35,13 @@ void visit_end_struct(Visitor *v)
     v->end_struct(v);
 }

-void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
+bool visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
                                  Error **errp)
 {
     if (v->start_implicit_struct) {
-        v->start_implicit_struct(v, obj, size, errp);
+        return v->start_implicit_struct(v, obj, size, errp);
     }
+    return false;
 }

 void visit_end_implicit_struct(Visitor *v)
@@ -50,9 +51,9 @@ void visit_end_implicit_struct(Visitor *v)
     }
 }

-void visit_start_list(Visitor *v, const char *name, Error **errp)
+bool visit_start_list(Visitor *v, const char *name, Error **errp)
 {
-    v->start_list(v, name, errp);
+    return v->start_list(v, name, errp);
 }

 GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp)
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 0270d25..2ad8fd6 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -125,7 +125,7 @@ static void qmp_input_pop(Visitor *v)
     qiv->nb_stack--;
 }

-static void qmp_input_start_struct(Visitor *v, void **obj, const char *kind,
+static bool qmp_input_start_struct(Visitor *v, void **obj, const char *kind,
                                    const char *name, size_t size, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
@@ -135,30 +135,34 @@ static void qmp_input_start_struct(Visitor *v, void **obj, const char *kind,
     if (!qobj || qobject_type(qobj) != QTYPE_QDICT) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "QDict");
-        return;
+        return false;
     }

     qmp_input_push(qiv, qobj, &err);
     if (err) {
         error_propagate(errp, err);
-        return;
+        return false;
     }

     if (obj) {
         *obj = g_malloc0(size);
+        return true;
     }
+    return false;
 }


-static void qmp_input_start_implicit_struct(Visitor *v, void **obj,
+static bool qmp_input_start_implicit_struct(Visitor *v, void **obj,
                                             size_t size, Error **errp)
 {
     if (obj) {
         *obj = g_malloc0(size);
+        return true;
     }
+    return false;
 }

-static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
+static bool qmp_input_start_list(Visitor *v, const char *name, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
     QObject *qobj = qmp_input_get_object(qiv, name, true);
@@ -166,10 +170,11 @@ static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
     if (!qobj || qobject_type(qobj) != QTYPE_QLIST) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "list");
-        return;
+        return false;
     }

     qmp_input_push(qiv, qobj, errp);
+    return true;
 }

 static GenericList *qmp_input_next_list(Visitor *v, GenericList **list,
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index af1e01b..14fdb58 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -111,7 +111,7 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
     }
 }

-static void qmp_output_start_struct(Visitor *v, void **obj, const char *kind,
+static bool qmp_output_start_struct(Visitor *v, void **obj, const char *kind,
                                     const char *name, size_t unused,
                                     Error **errp)
 {
@@ -120,6 +120,7 @@ static void qmp_output_start_struct(Visitor *v, void **obj, const char *kind,

     qmp_output_add(qov, name, dict);
     qmp_output_push(qov, dict);
+    return false;
 }

 static void qmp_output_end_struct(Visitor *v)
@@ -128,13 +129,14 @@ static void qmp_output_end_struct(Visitor *v)
     qmp_output_pop(qov);
 }

-static void qmp_output_start_list(Visitor *v, const char *name, Error **errp)
+static bool qmp_output_start_list(Visitor *v, const char *name, Error **errp)
 {
     QmpOutputVisitor *qov = to_qov(v);
     QList *list = qlist_new();

     qmp_output_add(qov, name, list);
     qmp_output_push(qov, list);
+    return false;
 }

 static GenericList *qmp_output_next_list(Visitor *v, GenericList **listp,
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 5b2bc47..6e4dc89 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -117,7 +117,7 @@ error:
     siv->ranges = NULL;
 }

-static void
+static bool
 start_list(Visitor *v, const char *name, Error **errp)
 {
     StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
@@ -131,6 +131,7 @@ start_list(Visitor *v, const char *name, Error **errp)
             siv->cur = r->begin;
         }
     }
+    return true;
 }

 static GenericList *
diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index 54d4174..2d9207e 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -259,7 +259,7 @@ static void print_type_number(Visitor *v, double *obj, const char *name,
     string_output_set(sov, g_strdup_printf("%f", *obj));
 }

-static void
+static bool
 start_list(Visitor *v, const char *name, Error **errp)
 {
     StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
@@ -268,6 +268,7 @@ start_list(Visitor *v, const char *name, Error **errp)
     assert(sov->list_mode == LM_NONE);
     sov->list_mode = LM_STARTED;
     sov->head = true;
+    return false;
 }

 static GenericList *
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index c28208e..eeb6e67 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -58,8 +58,10 @@ def gen_visit_implicit_struct(typ):
 static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error **errp)
 {
     Error *err = NULL;
+    bool allocated;

-    visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err);
+    allocated = visit_start_implicit_struct(v, (void **)obj,
+                                            sizeof(%(c_type)s), &err);
     if (err) {
         goto out;
     }
@@ -69,6 +71,10 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error *
     visit_type_%(c_type)s_fields(v, obj, &err);
 out_obj:
     visit_end_implicit_struct(v);
+    if (allocated && err) {
+        g_free(*obj);
+        *obj = NULL;
+    }
 out:
     error_propagate(errp, err);
 }
@@ -116,18 +122,15 @@ out:


 def gen_visit_list(name, element_type):
-    # FIXME: if *obj is NULL on entry, and the first visit_next_list()
-    # assigns to *obj, while a later one fails, we should clean up *obj
-    # rather than leaving it non-NULL. As currently written, the caller must
-    # call qapi_free_FOOList() to avoid a memory leak of the partial FOOList.
     return mcgen('''

 void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
 {
     Error *err = NULL;
     GenericList *i, **prev;
+    bool allocated;

-    visit_start_list(v, name, &err);
+    allocated = visit_start_list(v, name, &err);
     if (err) {
         goto out;
     }
@@ -141,6 +144,10 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error

     visit_end_list(v);
 out:
+    if (allocated && err) {
+        qapi_free_%(c_name)s(*obj);
+        *obj = NULL;
+    }
     error_propagate(errp, err);
 }
 ''',
@@ -171,8 +178,10 @@ def gen_visit_alternate(name, variants):
 void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
 {
     Error *err = NULL;
+    bool allocated;

-    visit_start_implicit_struct(v, (void**) obj, sizeof(%(c_name)s), &err);
+    allocated = visit_start_implicit_struct(v, (void **)obj,
+                                            sizeof(%(c_name)s), &err);
     if (err) {
         goto out;
     }
@@ -201,11 +210,15 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
     }
 out_obj:
     visit_end_implicit_struct(v);
+    if (allocated && err) {
+        qapi_free_%(c_name)s(*obj);
+        *obj = NULL;
+    }
 out:
     error_propagate(errp, err);
 }
 ''',
-                 name=name)
+                 name=name, c_name=c_name(name))

     return ret

@@ -233,8 +246,10 @@ def gen_visit_object(name, base, members, variants):
 void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
 {
     Error *err = NULL;
+    bool allocated;

-    visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
+    allocated = visit_start_struct(v, (void **)obj, "%(name)s",
+                                   name, sizeof(%(c_name)s), &err);
     if (err) {
         goto out;
     }
@@ -298,10 +313,15 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
     visit_check_struct(v, &err);
 out_obj:
     visit_end_struct(v);
+    if (allocated && err) {
+        qapi_free_%(c_name)s(*obj);
+        *obj = NULL;
+    }
 out:
     error_propagate(errp, err);
 }
-''')
+''',
+                 c_name=c_name(name))

     return ret

diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index b132775..f03cdf5 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -227,14 +227,13 @@ static void test_dealloc_partial(void)
         QDECREF(ud2_dict);
     }

-    /* verify partial success */
-    assert(ud2 != NULL);
-    assert(ud2->string0 != NULL);
-    assert(strcmp(ud2->string0, text) == 0);
-    assert(ud2->dict1 == NULL);
-
-    /* confirm & release construction error */
+    /* verify that visit_type_XXX() cleans up properly on error */
     error_free_or_abort(&err);
+    assert(!ud2);
+
+    /* Manually create a partial object, leaving ud2->dict1 at NULL */
+    ud2 = g_new0(UserDefTwo, 1);
+    ud2->string0 = g_strdup(text);

     /* tear down partial object */
     qapi_free_UserDefTwo(ud2);
diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
index f1c2e3b..9792277 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -181,10 +181,7 @@ static void test_validate_fail_struct(TestInputVisitorData *data,

     visit_type_TestStruct(v, &p, NULL, &err);
     error_free_or_abort(&err);
-    if (p) {
-        g_free(p->string);
-    }
-    g_free(p);
+    g_assert(!p);
 }

 static void test_validate_fail_struct_nested(TestInputVisitorData *data,
@@ -198,7 +195,7 @@ static void test_validate_fail_struct_nested(TestInputVisitorData *data,

     visit_type_UserDefTwo(v, &udp, NULL, &err);
     error_free_or_abort(&err);
-    qapi_free_UserDefTwo(udp);
+    g_assert(!udp);
 }

 static void test_validate_fail_list(TestInputVisitorData *data,
@@ -212,7 +209,7 @@ static void test_validate_fail_list(TestInputVisitorData *data,

     visit_type_UserDefOneList(v, &head, NULL, &err);
     error_free_or_abort(&err);
-    qapi_free_UserDefOneList(head);
+    g_assert(!head);
 }

 static void test_validate_fail_union_native_list(TestInputVisitorData *data,
@@ -227,7 +224,7 @@ static void test_validate_fail_union_native_list(TestInputVisitorData *data,

     visit_type_UserDefNativeListUnion(v, &tmp, NULL, &err);
     error_free_or_abort(&err);
-    qapi_free_UserDefNativeListUnion(tmp);
+    g_assert(!tmp);
 }

 static void test_validate_fail_union_flat(TestInputVisitorData *data,
@@ -241,7 +238,7 @@ static void test_validate_fail_union_flat(TestInputVisitorData *data,

     visit_type_UserDefFlatUnion(v, &tmp, NULL, &err);
     error_free_or_abort(&err);
-    qapi_free_UserDefFlatUnion(tmp);
+    g_assert(!tmp);
 }

 static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data,
@@ -256,13 +253,13 @@ static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data,

     visit_type_UserDefFlatUnion2(v, &tmp, NULL, &err);
     error_free_or_abort(&err);
-    qapi_free_UserDefFlatUnion2(tmp);
+    g_assert(!tmp);
 }

 static void test_validate_fail_alternate(TestInputVisitorData *data,
                                          const void *unused)
 {
-    UserDefAlternate *tmp = NULL;
+    UserDefAlternate *tmp;
     Visitor *v;
     Error *err = NULL;

@@ -270,7 +267,7 @@ static void test_validate_fail_alternate(TestInputVisitorData *data,

     visit_type_UserDefAlternate(v, &tmp, NULL, &err);
     error_free_or_abort(&err);
-    qapi_free_UserDefAlternate(tmp);
+    g_assert(!tmp);
 }

 static void do_test_validate_qmp_introspect(TestInputVisitorData *data,
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index b4a5bee..45d06f3 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -708,18 +708,12 @@ static void test_visitor_in_errors(TestInputVisitorData *data,

     visit_type_TestStruct(v, &p, NULL, &err);
     error_free_or_abort(&err);
-    /* FIXME - a failed parse should not leave a partially-allocated p
-     * for us to clean up; this could cause callers to leak memory. */
-    g_assert(p->string == NULL);
-
-    g_free(p->string);
-    g_free(p);
+    g_assert(!p);

     v = visitor_input_test_init(data, "[ '1', '2', false, '3' ]");
     visit_type_strList(v, &q, NULL, &err);
     error_free_or_abort(&err);
-    assert(q);
-    qapi_free_strList(q);
+    assert(!q);
 }

 static void test_visitor_in_wrong_type(TestInputVisitorData *data,
-- 
2.4.3

Re: [Qemu-devel] [PATCH v6 01/23] qapi: Make all visitors supply int64/uint64 callbacks

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Our qapi visitor contract supports multiple integer visitors:
> type_int (64-bit signed; mandatory), type_int64 (64-bit signed,
> optional), type_uint64 (64-bit unsigned, optional), and
> type_size (64-bit unsigned, optional, with the possibility of
> parsing differently than type_uint64 for the case of suffixed
> sizes).  But the default code treats any implementation without
> type_uint64 as just falling back on type_int, which can cause
> confusing results for values larger than LLONG_MAX (such as
> having to pass in a negative 2s complement value on input,
> and getting a negative result on output).

The visit_type_FOO() functions use their first argument's methods to do
their job.  For the integer visits, this involves picking the most
appropriate method from a set of candidates.  The set varies, because
some methods are optional.

Let me quickly review the state of things before this patch:

* visit_type_int() visits an int64_t and simply calls mandatory method
  type_int().

* The visit_type_intN() visit an intN_t.  They call optional method
  type_intN if it's there, else they use type_int() and check the result
  is in the type's range.

  Why optional type_int64() would ever differ from mandatory type_int()
  is anybody's guess.

* The visit_type_uintN() visit an uintN_t.  They call optional method
  type_uintN if it's there, else they use type_int() and check the
  result is in the type's range.  Except for type_uint64(), which simply
  converts the visited uint64_t to and from int64_t.  That's the issue
  you mentioned above.

* visit_type_size() visits an uint64_t.  It calls optional method
  type_size() if it's there, else optional type_uint64(), else
  type_int().  The latter again converts the visited uint64_t to and
  from int64_t.

Any change around here is prone to affect the weird cases, which always
confuse me, and that's why I'm going to read the patch very slowly.

> This patch does not fully address the disparity in parsing,
> but does move towards a cleaner situation where EVERY visitor
> provides both signed and unsigned variants as entry points;

Namely both type_int64() and type_uint64().

> then each client can either implement sane differences between
> the two, or document in place with a FIXME that there is
> munging going on for large values.  The next patch will then
> clean up the code to no longer allow conditional type_uint64.

I hope it also ditches type_int() as superfluous.

> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: new patch, but stems from v5 23/46
> ---
>  qapi/opts-visitor.c          |  5 +++--
>  qapi/qapi-dealloc-visitor.c  | 19 ++++++++++---------
>  qapi/qmp-input-visitor.c     | 23 ++++++++++++++++++++---
>  qapi/qmp-output-visitor.c    | 15 ++++++++++++---
>  qapi/string-input-visitor.c  | 22 +++++++++++++++++++---
>  qapi/string-output-visitor.c | 16 +++++++++++++---
>  6 files changed, 77 insertions(+), 23 deletions(-)
>
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index ef5fb8b..bc2b976 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -354,7 +354,7 @@ opts_type_bool(Visitor *v, bool *obj, const char *name, Error **errp)
>
>
>  static void
> -opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
> +opts_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp)
>  {
>      OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>      const QemuOpt *opt;
> @@ -522,7 +522,8 @@ opts_visitor_new(const QemuOpts *opts)
>       */
>      ov->visitor.type_enum = &input_type_enum;
>
> -    ov->visitor.type_int    = &opts_type_int;
> +    ov->visitor.type_int    = &opts_type_int64;
> +    ov->visitor.type_int64  = &opts_type_int64;
>      ov->visitor.type_uint64 = &opts_type_uint64;
>      ov->visitor.type_size   = &opts_type_size;
>      ov->visitor.type_bool   = &opts_type_bool;

Straightforward.  Note that type_int == type_int64 now.  Same for the
other visitors, actually.

> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index 737deab..5d1b2e6 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -136,8 +136,13 @@ static void qapi_dealloc_type_str(Visitor *v, char **obj, const char *name,
>      }
>  }
>
> -static void qapi_dealloc_type_int(Visitor *v, int64_t *obj, const char *name,
> -                                  Error **errp)
> +static void qapi_dealloc_type_int64(Visitor *v, int64_t *obj,
> +                                    const char *name, Error **errp)
> +{
> +}
> +
> +static void qapi_dealloc_type_uint64(Visitor *v, uint64_t *obj,
> +                                     const char *name, Error **errp)
>  {
>  }
>
> @@ -159,11 +164,6 @@ static void qapi_dealloc_type_anything(Visitor *v, QObject **obj,
>      }
>  }
>
> -static void qapi_dealloc_type_size(Visitor *v, uint64_t *obj, const char *name,
> -                                   Error **errp)
> -{
> -}
> -
>  static void qapi_dealloc_type_enum(Visitor *v, int *obj,
>                                     const char * const strings[],
>                                     const char *kind, const char *name,
> @@ -220,12 +220,13 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
>      v->visitor.next_list = qapi_dealloc_next_list;
>      v->visitor.end_list = qapi_dealloc_end_list;
>      v->visitor.type_enum = qapi_dealloc_type_enum;
> -    v->visitor.type_int = qapi_dealloc_type_int;
> +    v->visitor.type_int = qapi_dealloc_type_int64;
> +    v->visitor.type_int64 = qapi_dealloc_type_int64;
> +    v->visitor.type_uint64 = qapi_dealloc_type_uint64;
>      v->visitor.type_bool = qapi_dealloc_type_bool;
>      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.type_size = qapi_dealloc_type_size;
>      v->visitor.start_union = qapi_dealloc_start_union;
>
>      QTAILQ_INIT(&v->stack);

Same straightforward change to make type_int == type_int64.

Additionally, define type_uint64() instead of type_size().  No change
for visit_type_size().  visit_type_uint64() will now call type_uint64()
to do nothing instead of type_int().  Good.

> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 932b5f3..96dafcb 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -224,8 +224,23 @@ static void qmp_input_get_next_type(Visitor *v, QType *type, bool promote_int,
>      }
>  }
>
> -static void qmp_input_type_int(Visitor *v, int64_t *obj, const char *name,
> -                               Error **errp)
> +static void qmp_input_type_int64(Visitor *v, int64_t *obj, const char *name,
> +                                 Error **errp)
> +{
> +    QmpInputVisitor *qiv = to_qiv(v);
> +    QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true));
> +
> +    if (!qint) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> +                   "integer");
> +        return;
> +    }
> +
> +    *obj = qint_get_int(qint);
> +}
> +
> +static void qmp_input_type_uint64(Visitor *v, uint64_t *obj, const char *name,
> +                                  Error **errp)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
>      QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true));

Diff obscures what actually happens here, namely:

* Rename qmp_input_type_int() to qmp_input_type_int64().

* New qmp_input_type_uint64(), only difference to qmp_input_type_int64()
  is uint64_t instead of int64_t.  Note that QInt can only hold int64_t,
  so this still converts from int64_t to uint64_t, except it now does it
  in qmp_input_type_uint64() instead of visit_type_uint64().  Worth a
  FIXME?

> @@ -341,7 +356,9 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
>      v->visitor.next_list = qmp_input_next_list;
>      v->visitor.end_list = qmp_input_end_list;
>      v->visitor.type_enum = input_type_enum;
> -    v->visitor.type_int = qmp_input_type_int;
> +    v->visitor.type_int = qmp_input_type_int64;
> +    v->visitor.type_int64 = qmp_input_type_int64;
> +    v->visitor.type_uint64 = qmp_input_type_uint64;
>      v->visitor.type_bool = qmp_input_type_bool;
>      v->visitor.type_str = qmp_input_type_str;
>      v->visitor.type_number = qmp_input_type_number;
> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> index 29899ac..a52f26f 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -158,8 +158,15 @@ static void qmp_output_end_list(Visitor *v, Error **errp)
>      qmp_output_pop(qov);
>  }
>
> -static void qmp_output_type_int(Visitor *v, int64_t *obj, const char *name,
> -                                Error **errp)
> +static void qmp_output_type_int64(Visitor *v, int64_t *obj, const char *name,
> +                                  Error **errp)
> +{
> +    QmpOutputVisitor *qov = to_qov(v);
> +    qmp_output_add(qov, name, qint_from_int(*obj));
> +}
> +
> +static void qmp_output_type_uint64(Visitor *v, uint64_t *obj, const char *name,
> +                                   Error **errp)
>  {
>      QmpOutputVisitor *qov = to_qov(v);
>      qmp_output_add(qov, name, qint_from_int(*obj));

Again, diff obscures the actual change:

* Rename qmp_output_type_int() to qmp_output_type_int64().

* New qmp_output_type_uint64(), like qmp_output_type_int64(), but with
  uint64_t instead of int64_t.  As above, this still converts from
  uint64_t to int64_t, except it now does it in qmp_output_type_int64()
  instead of visit_type_uint64().  Worth a FIXME?

> @@ -241,7 +248,9 @@ QmpOutputVisitor *qmp_output_visitor_new(void)
>      v->visitor.next_list = qmp_output_next_list;
>      v->visitor.end_list = qmp_output_end_list;
>      v->visitor.type_enum = output_type_enum;
> -    v->visitor.type_int = qmp_output_type_int;
> +    v->visitor.type_int = qmp_output_type_int64;
> +    v->visitor.type_int64 = qmp_output_type_int64;
> +    v->visitor.type_uint64 = qmp_output_type_uint64;
>      v->visitor.type_bool = qmp_output_type_bool;
>      v->visitor.type_str = qmp_output_type_str;
>      v->visitor.type_number = qmp_output_type_number;
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index dee780a..0931321 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -179,8 +179,8 @@ end_list(Visitor *v, Error **errp)
>      siv->head = true;
>  }
>
> -static void parse_type_int(Visitor *v, int64_t *obj, const char *name,
> -                           Error **errp)
> +static void parse_type_int64(Visitor *v, int64_t *obj, const char *name,
> +                             Error **errp)
>  {
>      StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
>
> @@ -221,6 +221,20 @@ error:
>                 "an int64 value or range");
>  }
>
> +static void parse_type_uint64(Visitor *v, uint64_t *obj, const char *name,
> +                              Error **errp)
> +{
> +    /* FIXME - don't handle numbers > INT64_MAX as negative */
> +    int64_t i;
> +    Error *err = NULL;

Blank line here, please.

> +    parse_type_int64(v, &i, name, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +    } else {
> +        *obj = i;
> +    }
> +}
> +
>  static void parse_type_size(Visitor *v, uint64_t *obj, const char *name,
>                              Error **errp)
>  {
> @@ -330,7 +344,9 @@ StringInputVisitor *string_input_visitor_new(const char *str)
>      v = g_malloc0(sizeof(*v));
>
>      v->visitor.type_enum = input_type_enum;
> -    v->visitor.type_int = parse_type_int;
> +    v->visitor.type_int = parse_type_int64;
> +    v->visitor.type_int64 = parse_type_int64;
> +    v->visitor.type_uint64 = parse_type_uint64;
>      v->visitor.type_size = parse_type_size;
>      v->visitor.type_bool = parse_type_bool;
>      v->visitor.type_str = parse_type_str;
> diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> index b86ce2c..83ca6cc 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -116,8 +116,8 @@ static void format_string(StringOutputVisitor *sov, Range *r, bool next,
>      }
>  }
>
> -static void print_type_int(Visitor *v, int64_t *obj, const char *name,
> -                           Error **errp)
> +static void print_type_int64(Visitor *v, int64_t *obj, const char *name,
> +                             Error **errp)
>  {
>      StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
>      GList *l;
> @@ -192,6 +192,14 @@ static void print_type_int(Visitor *v, int64_t *obj, const char *name,
>      }
>  }
>
> +static void print_type_uint64(Visitor *v, uint64_t *obj, const char *name,
> +                             Error **errp)
> +{
> +    /* FIXME - don't handle numbers > INT64_MAX as negative */
> +    int64_t i = *obj;
> +    print_type_int64(v, &i, name, errp);
> +}
> +
>  static void print_type_size(Visitor *v, uint64_t *obj, const char *name,
>                             Error **errp)
>  {
> @@ -340,7 +348,9 @@ StringOutputVisitor *string_output_visitor_new(bool human)
>      v->string = g_string_new(NULL);
>      v->human = human;
>      v->visitor.type_enum = output_type_enum;
> -    v->visitor.type_int = print_type_int;
> +    v->visitor.type_int = print_type_int64;
> +    v->visitor.type_int64 = print_type_int64;
> +    v->visitor.type_uint64 = print_type_uint64;
>      v->visitor.type_size = print_type_size;
>      v->visitor.type_bool = print_type_bool;
>      v->visitor.type_str = print_type_str;

Re: [Qemu-devel] [PATCH v6 02/23] qapi: Require int64/uint64 implementation

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Now that all visitors supply both type_int64() and type_uint64()
> callbacks, we can drop the redundant type_int() callback (the
> public interface visit_type_int() remains, but calls into
> type_int64() under the hood).
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: new patch, but stems from v5 23/46
> ---
>  include/qapi/visitor-impl.h  |  1 -
>  qapi/opts-visitor.c          |  1 -
>  qapi/qapi-dealloc-visitor.c  |  1 -
>  qapi/qapi-visit-core.c       | 50 ++++++++++++++------------------------------
>  qapi/qmp-input-visitor.c     |  1 -
>  qapi/qmp-output-visitor.c    |  1 -
>  qapi/string-input-visitor.c  |  1 -
>  qapi/string-output-visitor.c |  1 -
>  8 files changed, 16 insertions(+), 41 deletions(-)
>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 44a21b7..70326e0 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -36,7 +36,6 @@ struct Visitor
>      void (*get_next_type)(Visitor *v, QType *type, bool promote_int,
>                            const char *name, Error **errp);
>
> -    void (*type_int)(Visitor *v, int64_t *obj, const char *name, Error **errp);
>      void (*type_bool)(Visitor *v, bool *obj, const char *name, Error **errp);
>      void (*type_str)(Visitor *v, char **obj, const char *name, Error **errp);
>      void (*type_number)(Visitor *v, double *obj, const char *name,
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index bc2b976..8104e42 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -522,7 +522,6 @@ opts_visitor_new(const QemuOpts *opts)
>       */
>      ov->visitor.type_enum = &input_type_enum;
>
> -    ov->visitor.type_int    = &opts_type_int64;
>      ov->visitor.type_int64  = &opts_type_int64;
>      ov->visitor.type_uint64 = &opts_type_uint64;
>      ov->visitor.type_size   = &opts_type_size;
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index 5d1b2e6..5133d37 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -220,7 +220,6 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
>      v->visitor.next_list = qapi_dealloc_next_list;
>      v->visitor.end_list = qapi_dealloc_end_list;
>      v->visitor.type_enum = qapi_dealloc_type_enum;
> -    v->visitor.type_int = qapi_dealloc_type_int64;
>      v->visitor.type_int64 = qapi_dealloc_type_int64;
>      v->visitor.type_uint64 = qapi_dealloc_type_uint64;
>      v->visitor.type_bool = qapi_dealloc_type_bool;
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 6d63e40..88bed9c 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -97,19 +97,19 @@ void visit_type_enum(Visitor *v, int *obj, const char * const strings[],
>
>  void visit_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
>  {
> -    v->type_int(v, obj, name, errp);
> +    v->type_int64(v, obj, name, errp);
>  }
>
>  void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp)
>  {
> -    int64_t value;
> +    uint64_t value;
>
>      if (v->type_uint8) {
>          v->type_uint8(v, obj, name, errp);
>      } else {
>          value = *obj;
> -        v->type_int(v, &value, name, errp);
> -        if (value < 0 || value > UINT8_MAX) {
> +        v->type_uint64(v, &value, name, errp);
> +        if (value > UINT8_MAX) {
>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                         name ? name : "null", "uint8_t");
>              return;

Note that this relies on value being in range after type_uint64() fails.
If it isn't, we call error_setg() with non-null *errp.

Two solutions:

1. Stipulate that type_uint64() & friends leave value alone on error.
   Works, because its initial value *obj is in range.

2. Avoid using value on error.  A clean way to do this:

        Error *err = NULL;

        value = *obj;
        v->type_uint64(v, &value, name, &err);
        if (err) {
            error_propagate(errp, err);
            return;
        }
        if (value < 0 || value > UINT8_MAX) {
            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                       name ? name : "null", "uint8_t");
            return;
        }
        *obj = value;

   More boilerplate.  If we pick this solution, we'll want a separate
   PATCH 1.5 cleaning up the preexisting instances.

> @@ -120,14 +120,14 @@ void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp)
>
>  void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Error **errp)
>  {
> -    int64_t value;
> +    uint64_t value;
>
>      if (v->type_uint16) {
>          v->type_uint16(v, obj, name, errp);
>      } else {
>          value = *obj;
> -        v->type_int(v, &value, name, errp);
> -        if (value < 0 || value > UINT16_MAX) {
> +        v->type_uint64(v, &value, name, errp);
> +        if (value > UINT16_MAX) {
>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                         name ? name : "null", "uint16_t");
>              return;

Good.  The fewer signed-unsigned conversions, the better.

[...]

Re: [Qemu-devel] [PATCH v6 03/23] qapi: Consolidate visitor integer callbacks

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Commit 4e27e819 introduced optional visitor callbacks for all
> sorts of int types, but except for type_uint64() and type_size(),
> none of them have ever been supplied (the generic implementation
> based on using type_[u]int64() then bounds-checking works just
> fine). In the interest of simplicity, it's easier to make the
> visitor callback interface not have to worry about the other
> sizes.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> v6: split off from v5 23/46
> original version also appeared in v6-v9 of subset D
> ---
>  include/qapi/visitor-impl.h |  22 ++++-----
>  qapi/qapi-visit-core.c      | 117 ++++++++++++++++++--------------------------
>  2 files changed, 59 insertions(+), 80 deletions(-)
>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 70326e0..d071c06 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -1,7 +1,7 @@
>  /*
>   * Core Definitions for QAPI Visitor implementations
>   *
> - * Copyright (C) 2012 Red Hat, Inc.
> + * Copyright (C) 2012, 2015 Red Hat, Inc.
>   *
>   * Author: Paolo Bonizni <pbonzini@redhat.com>
>   *
> @@ -36,6 +36,16 @@ struct Visitor
>      void (*get_next_type)(Visitor *v, QType *type, bool promote_int,
>                            const char *name, Error **errp);
>
> +    /* Must be set. */
> +    void (*type_int64)(Visitor *v, int64_t *obj, const char *name,
> +                       Error **errp);
> +    /* Must be set. */
> +    void (*type_uint64)(Visitor *v, uint64_t *obj, const char *name,
> +                        Error **errp);
> +    /* Only required to visit sizes differently than (*type_uint64)().  */

What about:

       /* Optional; fallback is type_uint64() */

> +    void (*type_size)(Visitor *v, uint64_t *obj, const char *name,
> +                      Error **errp);
> +    /* Must be set. */
>      void (*type_bool)(Visitor *v, bool *obj, const char *name, Error **errp);
>      void (*type_str)(Visitor *v, char **obj, const char *name, Error **errp);
>      void (*type_number)(Visitor *v, double *obj, const char *name,
> @@ -46,16 +56,6 @@ struct Visitor
>      /* May be NULL; most useful for input visitors. */
>      void (*optional)(Visitor *v, bool *present, const char *name);
>
> -    void (*type_uint8)(Visitor *v, uint8_t *obj, const char *name, Error **errp);
> -    void (*type_uint16)(Visitor *v, uint16_t *obj, const char *name, Error **errp);
> -    void (*type_uint32)(Visitor *v, uint32_t *obj, const char *name, Error **errp);
> -    void (*type_uint64)(Visitor *v, uint64_t *obj, const char *name, Error **errp);
> -    void (*type_int8)(Visitor *v, int8_t *obj, const char *name, Error **errp);
> -    void (*type_int16)(Visitor *v, int16_t *obj, const char *name, Error **errp);
> -    void (*type_int32)(Visitor *v, int32_t *obj, const char *name, Error **errp);
> -    void (*type_int64)(Visitor *v, int64_t *obj, const char *name, Error **errp);
> -    /* visit_type_size() falls back to (*type_uint64)() if type_size is unset */
> -    void (*type_size)(Visitor *v, uint64_t *obj, const char *name, Error **errp);
>      bool (*start_union)(Visitor *v, bool data_present, Error **errp);
>      void (*end_union)(Visitor *v, bool data_present, Error **errp);
>  };
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 88bed9c..be1fcdd 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -104,57 +104,48 @@ void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp)
>  {
>      uint64_t value;
>
> -    if (v->type_uint8) {
> -        v->type_uint8(v, obj, name, errp);
> -    } else {
> -        value = *obj;
> -        v->type_uint64(v, &value, name, errp);
> -        if (value > UINT8_MAX) {
> -            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> -                       name ? name : "null", "uint8_t");
> -            return;
> -        }
> -        *obj = value;
> +    value = *obj;
> +    v->type_uint64(v, &value, name, errp);
> +    if (value > UINT8_MAX) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> +                   name ? name : "null", "uint8_t");
> +        return;
>      }
> +    *obj = value;
>  }

Best reviewed with whitespace ignored.

Have you considered doing this before PATCH 02?  Hmm, swapping them now
is probably not worth the bother.

[...]

Re: [Qemu-devel] [PATCH v6 05/23] qmp: Fix reference-counting of qnull on empty output visit

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Commit 6c2f9a15 ensured that we would not return NULL when the
> caller used an output visitor but had nothing to visit. But
> in doing so, it added a FIXME about a reference count leak
> that could abort qemu in the (unlikely) case of SIZE_MAX such
> visits (more plausible on 32-bit).

The commit message says "We'll want to fix it for real before the
release."  Is this patch for 2.5?  For what it's worth, it applies to
master.

> This fixes things by documenting the internal contracts, and
> explaining why the internal function can return NULL and only
> the public facing interface needs to worry about qnull(),
> thus avoiding over-referencing the qnull_ global object.
>
> It does not, however, fix the stupidity of the stack mixing
> up two separate pieces of information; add a FIXME to explain
> that issue.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: no change
> ---
>  qapi/qmp-output-visitor.c       | 30 ++++++++++++++++++++++++++++--
>  tests/test-qmp-output-visitor.c |  2 ++
>  2 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> index e66ab3c..9d0f9d1 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -29,6 +29,12 @@ typedef QTAILQ_HEAD(QStack, QStackEntry) QStack;
>  struct QmpOutputVisitor
>  {
>      Visitor visitor;
> +    /* FIXME: we are abusing stack to hold two separate pieces of
> +     * information: the current root object, and the stack of objects
> +     * still being built.  Worse, our behavior is inconsistent:
> +     * visiting two top-level scalars in a row discards the first in
> +     * favor of the second, but visiting two top-level objects in a
> +     * row tries to append the second object into the first.  */

Uhh, I'll simply take your word for it.

>      QStack stack;
>  };
>
> @@ -41,10 +47,12 @@ static QmpOutputVisitor *to_qov(Visitor *v)
>      return container_of(v, QmpOutputVisitor, visitor);
>  }
>
> +/* Push @value onto the stack of current QObjects being built */
>  static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value)
>  {
>      QStackEntry *e = g_malloc0(sizeof(*e));
>
> +    assert(value);
>      e->value = value;

Keep in mind for later: we never push a null value on the stack, and
there is no other way to grow the stack.  Therefore, the stack holds
only non-null values.

>      if (qobject_type(e->value) == QTYPE_QLIST) {
>          e->is_list_head = true;
> @@ -52,16 +60,20 @@ static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value)
>      QTAILQ_INSERT_HEAD(&qov->stack, e, node);
>  }
>
> +/* Grab and remove the most recent QObject from the stack */
>  static QObject *qmp_output_pop(QmpOutputVisitor *qov)
>  {
>      QStackEntry *e = QTAILQ_FIRST(&qov->stack);
>      QObject *value;
> +
> +    assert(e);

I figure this assert the stack isn't empty.  Correct?

>      QTAILQ_REMOVE(&qov->stack, e, node);
>      value = e->value;
>      g_free(e);
>      return value;
>  }
>
> +/* Grab the root QObject, if any, in preparation to empty the stack */
>  static QObject *qmp_output_first(QmpOutputVisitor *qov)
>  {
>      QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack);
> @@ -72,24 +84,32 @@ static QObject *qmp_output_first(QmpOutputVisitor *qov)
       /*
        * FIXME Wrong, because qmp_output_get_qobject() will increment
        * the refcnt *again*.  We need to think through how visitors
>       * handle null.
>       */

Sure you don't want to drop the FIXME?

>      if (!e) {

I figure this means the stack is empty.  Now I wish I understood how we
use the stack.
    
> -        return qnull();
> +        /* No root */
> +        return NULL;
>      }
> -
> +    assert(e->value);

Safe because the stack holds only non-null values.

>      return e->value;
>  }

We return null exactly when the stack is empty (whatever that may mean).

>
> +/* Grab the most recent QObject from the stack, which must exist */
>  static QObject *qmp_output_last(QmpOutputVisitor *qov)
>  {
>      QStackEntry *e = QTAILQ_FIRST(&qov->stack);
> +
> +    assert(e);
>      return e->value;
>  }

Can't return null, because the stack holds only non-null values.

assert(e && e->value) to match qmp_output_first()?

>
> +/* Add @value to the current QObject being built.
> + * If the stack is visiting a dictionary or list, @value is now owned
> + * by that container. Otherwise, @value is now the root.  */
>  static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
>                                 QObject *value)
>  {
>      QObject *cur;
>
>      if (QTAILQ_EMPTY(&qov->stack)) {
> +        /* Stack was empty, track this object as root */
>          qmp_output_push_obj(qov, value);
>          return;
>      }
> @@ -98,13 +118,16 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
>
>      switch (qobject_type(cur)) {
>      case QTYPE_QDICT:
> +        assert(name);

Okay, but it really only converts one kind of crash into another one.

>          qdict_put_obj(qobject_to_qdict(cur), name, value);
>          break;
>      case QTYPE_QLIST:
>          qlist_append_obj(qobject_to_qlist(cur), value);
>          break;
>      default:
> +        /* The previous root was a scalar, replace it with a new root */
>          qobject_decref(qmp_output_pop(qov));
> +        assert(QTAILQ_EMPTY(&qov->stack));
>          qmp_output_push_obj(qov, value);
>          break;
>      }

Hand me the bucket.

> @@ -205,11 +228,14 @@ static void qmp_output_type_any(Visitor *v, QObject **obj, const char *name,
>      qmp_output_add_obj(qov, name, *obj);
>  }
>
> +/* Finish building, and return the root object. Will not be NULL. */
>  QObject *qmp_output_get_qobject(QmpOutputVisitor *qov)
>  {
>      QObject *obj = qmp_output_first(qov);
>      if (obj) {

Non-empty stack.

>          qobject_incref(obj);
> +    } else {

Empty stack.

> +        obj = qnull();
>      }
>      return obj;
>  }

Thanks for your comments.  They help a lot with understanding the code,
and they also demonstrate that it's muddled crap %-}

So, how does this contraption work?

A visitor cab encounter NULL only when it visits pointers (d'oh!).
Searching qapi-visit-core.c for **obj finds start_struct(),
start_implicit_struct(), type_str(), type_any().

As far as I can tell, start_implicit_struct() is for unboxed structs, so
NULL must not happen there.  This visitor doesn't implement it anyway.

qmp_output_type_str() substitutes "" for NULL.  Ooookay.

qmp_output_type_any() crashes on NULL.  Can this happen?

qmp_output_start_struct() ignores its obj argument.  My best guess is
that this substitutes an empty dictionary for NULL.

Okay, I give up: how do we get an empty stack and return qnull()?

> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index 3078442..8e6fc33 100644
> --- a/tests/test-qmp-output-visitor.c
> +++ b/tests/test-qmp-output-visitor.c
> @@ -461,6 +461,8 @@ static void test_visitor_out_empty(TestOutputVisitorData *data,
>
>      arg = qmp_output_get_qobject(data->qov);
>      g_assert(qobject_type(arg) == QTYPE_QNULL);
> +    /* Check that qnull reference counting is sane */
> +    g_assert(arg->refcnt == 2);
>      qobject_decref(arg);
>  }

Re: [Qemu-devel] [PATCH v6 02/23] qapi: Require int64/uint64 implementation

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3207 bytes --]

On 11/27/2015 05:05 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Now that all visitors supply both type_int64() and type_uint64()
>> callbacks, we can drop the redundant type_int() callback (the
>> public interface visit_type_int() remains, but calls into
>> type_int64() under the hood).
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>

>>  void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp)
>>  {
>> -    int64_t value;
>> +    uint64_t value;
>>
>>      if (v->type_uint8) {
>>          v->type_uint8(v, obj, name, errp);
>>      } else {
>>          value = *obj;
>> -        v->type_int(v, &value, name, errp);
>> -        if (value < 0 || value > UINT8_MAX) {
>> +        v->type_uint64(v, &value, name, errp);
>> +        if (value > UINT8_MAX) {
>>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>>                         name ? name : "null", "uint8_t");
>>              return;
> 
> Note that this relies on value being in range after type_uint64() fails.
> If it isn't, we call error_setg() with non-null *errp.
> 
> Two solutions:
> 
> 1. Stipulate that type_uint64() & friends leave value alone on error.
>    Works, because its initial value *obj is in range.

Pre-existing and simpler, but sets a poor example for the rest of the
code base (not everyone is going to read the fine print for why it works
here), and requires cross-file audits to ensure visitors comply.

> 
> 2. Avoid using value on error.  A clean way to do this:
> 
>         Error *err = NULL;
> 
>         value = *obj;
>         v->type_uint64(v, &value, name, &err);
>         if (err) {
>             error_propagate(errp, err);
>             return;
>         }
>         if (value < 0 || value > UINT8_MAX) {
>             error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                        name ? name : "null", "uint8_t");
>             return;
>         }
>         *obj = value;
> 
>    More boilerplate.  If we pick this solution, we'll want a separate
>    PATCH 1.5 cleaning up the preexisting instances.

Of course, if I do the cleanup as 1.5, then patch 3/23 reindents
everything, that's a lot of churn.  So I may end up rearranging 2 and 3
after all, and then do the cleanup as 3.5.

Or maybe option 3, write a pair of helper functions containing the
boilerplate for checking against min and max:

void visit_type_intN(Visitor *v, int64_t *obj, const char *name,
                     int64_t min, int64_t max, Error **errp);
void visit_type_uintN(Visitor *v, int64_t *obj, const char *name,
                      uint64_t max, Error **errp);

leaving us with simpler clients:

visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp)
{
    int64_t value = *obj;
    visit_type_uintN(v, &value, name, INT8_MIN, INT8_MAX, errp);
    *obj = value;
}

and here, because the helpers are in the same file, it's easier to prove
that value was unchanged on error.  Or I may even squash 2 and 3 into a
single patch now.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v6 05/23] qmp: Fix reference-counting of qnull on empty output visit

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 10735 bytes --]

On 11/27/2015 06:06 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Commit 6c2f9a15 ensured that we would not return NULL when the
>> caller used an output visitor but had nothing to visit. But
>> in doing so, it added a FIXME about a reference count leak
>> that could abort qemu in the (unlikely) case of SIZE_MAX such
>> visits (more plausible on 32-bit).
> 
> The commit message says "We'll want to fix it for real before the
> release."  Is this patch for 2.5?  For what it's worth, it applies to
> master.

It's a bit late for 2.5 now, and I don't think we'll run into 2^32 uses
of qnull(), so I can live with this waiting until 2.6 and additionally
adding qemu-stable in cc.

> 
>> This fixes things by documenting the internal contracts, and
>> explaining why the internal function can return NULL and only
>> the public facing interface needs to worry about qnull(),
>> thus avoiding over-referencing the qnull_ global object.
>>
>> It does not, however, fix the stupidity of the stack mixing
>> up two separate pieces of information; add a FIXME to explain
>> that issue.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> v6: no change
>> ---
>>  qapi/qmp-output-visitor.c       | 30 ++++++++++++++++++++++++++++--
>>  tests/test-qmp-output-visitor.c |  2 ++
>>  2 files changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
>> index e66ab3c..9d0f9d1 100644
>> --- a/qapi/qmp-output-visitor.c
>> +++ b/qapi/qmp-output-visitor.c
>> @@ -29,6 +29,12 @@ typedef QTAILQ_HEAD(QStack, QStackEntry) QStack;
>>  struct QmpOutputVisitor
>>  {
>>      Visitor visitor;
>> +    /* FIXME: we are abusing stack to hold two separate pieces of
>> +     * information: the current root object, and the stack of objects
>> +     * still being built.  Worse, our behavior is inconsistent:
>> +     * visiting two top-level scalars in a row discards the first in
>> +     * favor of the second, but visiting two top-level objects in a
>> +     * row tries to append the second object into the first.  */
> 
> Uhh, I'll simply take your word for it.

I clean it up in 6/23, if reviewing that one helps understand the
comment here.

> 
>>      QStack stack;
>>  };
>>
>> @@ -41,10 +47,12 @@ static QmpOutputVisitor *to_qov(Visitor *v)
>>      return container_of(v, QmpOutputVisitor, visitor);
>>  }
>>
>> +/* Push @value onto the stack of current QObjects being built */
>>  static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value)
>>  {
>>      QStackEntry *e = g_malloc0(sizeof(*e));
>>
>> +    assert(value);
>>      e->value = value;
> 
> Keep in mind for later: we never push a null value on the stack, and
> there is no other way to grow the stack.  Therefore, the stack holds
> only non-null values.
> 
>>      if (qobject_type(e->value) == QTYPE_QLIST) {
>>          e->is_list_head = true;
>> @@ -52,16 +60,20 @@ static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value)
>>      QTAILQ_INSERT_HEAD(&qov->stack, e, node);
>>  }
>>
>> +/* Grab and remove the most recent QObject from the stack */
>>  static QObject *qmp_output_pop(QmpOutputVisitor *qov)
>>  {
>>      QStackEntry *e = QTAILQ_FIRST(&qov->stack);
>>      QObject *value;
>> +
>> +    assert(e);
> 
> I figure this assert the stack isn't empty.  Correct?

Yes.

> 
>>      QTAILQ_REMOVE(&qov->stack, e, node);
>>      value = e->value;
>>      g_free(e);
>>      return value;
>>  }
>>
>> +/* Grab the root QObject, if any, in preparation to empty the stack */
>>  static QObject *qmp_output_first(QmpOutputVisitor *qov)
>>  {
>>      QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack);
>> @@ -72,24 +84,32 @@ static QObject *qmp_output_first(QmpOutputVisitor *qov)
>        /*
>         * FIXME Wrong, because qmp_output_get_qobject() will increment
>         * the refcnt *again*.  We need to think through how visitors
>>       * handle null.
>>       */
> 
> Sure you don't want to drop the FIXME?

Oops. I didn't rebase my patch quite correctly (when I first wrote it,
commit 6c2f9a15 wasn't yet in final form).  Yes, the comment is dead now.

> 
>>      if (!e) {
> 
> I figure this means the stack is empty.  Now I wish I understood how we
> use the stack.

The stack is either empty (visit just started) or the head contains a
root element (see qmp_output_add_obj calling qmp_output_push_obj).
Additionally, if the head is a QDict or QList, then the 2nd through
(N+1)th elements are the 1st through Nth incomplete containers still
collecting members.

>     
>> -        return qnull();
>> +        /* No root */
>> +        return NULL;
>>      }
>> -
>> +    assert(e->value);
> 
> Safe because the stack holds only non-null values.
> 
>>      return e->value;
>>  }
> 
> We return null exactly when the stack is empty (whatever that may mean).
> 
>>
>> +/* Grab the most recent QObject from the stack, which must exist */
>>  static QObject *qmp_output_last(QmpOutputVisitor *qov)
>>  {
>>      QStackEntry *e = QTAILQ_FIRST(&qov->stack);
>> +
>> +    assert(e);
>>      return e->value;
>>  }
> 
> Can't return null, because the stack holds only non-null values.
> 
> assert(e && e->value) to match qmp_output_first()?

Can't hurt.

> 
>>
>> +/* Add @value to the current QObject being built.
>> + * If the stack is visiting a dictionary or list, @value is now owned
>> + * by that container. Otherwise, @value is now the root.  */
>>  static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
>>                                 QObject *value)
>>  {
>>      QObject *cur;
>>
>>      if (QTAILQ_EMPTY(&qov->stack)) {
>> +        /* Stack was empty, track this object as root */
>>          qmp_output_push_obj(qov, value);
>>          return;
>>      }
>> @@ -98,13 +118,16 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
>>
>>      switch (qobject_type(cur)) {
>>      case QTYPE_QDICT:
>> +        assert(name);
> 
> Okay, but it really only converts one kind of crash into another one.
> 
>>          qdict_put_obj(qobject_to_qdict(cur), name, value);
>>          break;
>>      case QTYPE_QLIST:
>>          qlist_append_obj(qobject_to_qlist(cur), value);
>>          break;
>>      default:
>> +        /* The previous root was a scalar, replace it with a new root */
>>          qobject_decref(qmp_output_pop(qov));
>> +        assert(QTAILQ_EMPTY(&qov->stack));
>>          qmp_output_push_obj(qov, value);
>>          break;
>>      }
> 
> Hand me the bucket.

Yep, this was the gross abuse of the stack that the FIXME above tries to
point out, and which 6/23 tries to clean up.

> 
>> @@ -205,11 +228,14 @@ static void qmp_output_type_any(Visitor *v, QObject **obj, const char *name,
>>      qmp_output_add_obj(qov, name, *obj);
>>  }
>>
>> +/* Finish building, and return the root object. Will not be NULL. */
>>  QObject *qmp_output_get_qobject(QmpOutputVisitor *qov)
>>  {
>>      QObject *obj = qmp_output_first(qov);
>>      if (obj) {
> 
> Non-empty stack.
> 
>>          qobject_incref(obj);
>> +    } else {
> 
> Empty stack.
> 
>> +        obj = qnull();
>>      }
>>      return obj;
>>  }
> 
> Thanks for your comments.  They help a lot with understanding the code,
> and they also demonstrate that it's muddled crap %-}

Indeed.

> 
> So, how does this contraption work?
> 
> A visitor cab encounter NULL only when it visits pointers (d'oh!).
> Searching qapi-visit-core.c for **obj finds start_struct(),
> start_implicit_struct(), type_str(), type_any().
> 
> As far as I can tell, start_implicit_struct() is for unboxed structs, so
> NULL must not happen there.

You are correct that start_implicit_struct is for unboxed substructs
(the branch of a flat union).  And we should never get here with it
being NULL (although until commit 20/23 of this series, it was because
we are abusing the 'data' member of the union during visit_start_union()).

>  This visitor doesn't implement it anyway.

Well, not yet (but it IS on queue as a patch by Zoltán:
http://repo.or.cz/qemu/ericb.git/commitdiff/9ee9f4ad)

> 
> qmp_output_type_str() substitutes "" for NULL.  Ooookay.

Not the smartest thing we've done, but don't know how hard it would be
to fix (we DO have clients abusing this aspect of qapi that would have
to be cleaned up to quit passing in incomplete qapi structs).  Someday
we may want to actually use qnull() instead of qstring("") (so that the
JSON on the wire would read "name":null instead of "name":"").  But if
we make that change, we would still be inserting a non-NULL QObject onto
the stack.

> 
> qmp_output_type_any() crashes on NULL.  Can this happen?

Again, if the QObject is trying to represent NULL, it does so with
qnull() (a non-null QObject), so we should never pass in NULL.  We
aren't using 'any' very heavily, so I doubt we have any broken clients.

> 
> qmp_output_start_struct() ignores its obj argument.  My best guess is
> that this substitutes an empty dictionary for NULL.

No one should ever be passing in NULL for a QDict, but yes, the code
would create an empty QDict on output if that happened.  Maybe worth
adding an assert; but not in this patch, because I don't know if we have
broken clients.

> 
> Okay, I give up: how do we get an empty stack and return qnull()?

The only way to get an empty stack is to never visit anything in the
first place.  See test-qmp-output-visitor.c:test_visitor_out_empty(),
which creates a visitor with qmp_output_get_qobject() then immediately
asks for the root object even though it didn't call any visit_type_FOO()
to populate a root object.  And my recollection is that back in
September you had an example use of qom-get that could trigger a failure
early enough to run into the empty stack scenario, which is why you
added qnull() in the first place.

> 
>> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
>> index 3078442..8e6fc33 100644
>> --- a/tests/test-qmp-output-visitor.c
>> +++ b/tests/test-qmp-output-visitor.c
>> @@ -461,6 +461,8 @@ static void test_visitor_out_empty(TestOutputVisitorData *data,
>>
>>      arg = qmp_output_get_qobject(data->qov);
>>      g_assert(qobject_type(arg) == QTYPE_QNULL);
>> +    /* Check that qnull reference counting is sane */
>> +    g_assert(arg->refcnt == 2);
>>      qobject_decref(arg);
>>  }
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v6 02/23] qapi: Require int64/uint64 implementation

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/27/2015 05:05 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> Now that all visitors supply both type_int64() and type_uint64()
>>> callbacks, we can drop the redundant type_int() callback (the
>>> public interface visit_type_int() remains, but calls into
>>> type_int64() under the hood).
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>>
>
>>>  void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp)
>>>  {
>>> -    int64_t value;
>>> +    uint64_t value;
>>>
>>>      if (v->type_uint8) {
>>>          v->type_uint8(v, obj, name, errp);
>>>      } else {
>>>          value = *obj;
>>> -        v->type_int(v, &value, name, errp);
>>> -        if (value < 0 || value > UINT8_MAX) {
>>> +        v->type_uint64(v, &value, name, errp);
>>> +        if (value > UINT8_MAX) {
>>>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>>>                         name ? name : "null", "uint8_t");
>>>              return;
>> 
>> Note that this relies on value being in range after type_uint64() fails.
>> If it isn't, we call error_setg() with non-null *errp.
>> 
>> Two solutions:
>> 
>> 1. Stipulate that type_uint64() & friends leave value alone on error.
>>    Works, because its initial value *obj is in range.
>
> Pre-existing and simpler, but sets a poor example for the rest of the
> code base (not everyone is going to read the fine print for why it works
> here), and requires cross-file audits to ensure visitors comply.

Yes, but "don't mess up externally visible state on error" is a pretty
common design maxim.

>> 2. Avoid using value on error.  A clean way to do this:
>> 
>>         Error *err = NULL;
>> 
>>         value = *obj;
>>         v->type_uint64(v, &value, name, &err);
>>         if (err) {
>>             error_propagate(errp, err);
>>             return;
>>         }
>>         if (value < 0 || value > UINT8_MAX) {
>>             error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>>                        name ? name : "null", "uint8_t");
>>             return;
>>         }
>>         *obj = value;
>> 
>>    More boilerplate.  If we pick this solution, we'll want a separate
>>    PATCH 1.5 cleaning up the preexisting instances.
>
> Of course, if I do the cleanup as 1.5, then patch 3/23 reindents
> everything, that's a lot of churn.  So I may end up rearranging 2 and 3
> after all, and then do the cleanup as 3.5.
>
> Or maybe option 3, write a pair of helper functions containing the
> boilerplate for checking against min and max:
>
> void visit_type_intN(Visitor *v, int64_t *obj, const char *name,
>                      int64_t min, int64_t max, Error **errp);
> void visit_type_uintN(Visitor *v, int64_t *obj, const char *name,
>                       uint64_t max, Error **errp);
>
> leaving us with simpler clients:
>
> visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp)
> {
>     int64_t value = *obj;
>     visit_type_uintN(v, &value, name, INT8_MIN, INT8_MAX, errp);
>     *obj = value;
> }
>
> and here, because the helpers are in the same file, it's easier to prove
> that value was unchanged on error.  Or I may even squash 2 and 3 into a
> single patch now.

Artistic license applies.

Re: [Qemu-devel] [PATCH v6 05/23] qmp: Fix reference-counting of qnull on empty output visit

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/27/2015 06:06 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> Commit 6c2f9a15 ensured that we would not return NULL when the
>>> caller used an output visitor but had nothing to visit. But
>>> in doing so, it added a FIXME about a reference count leak
>>> that could abort qemu in the (unlikely) case of SIZE_MAX such
>>> visits (more plausible on 32-bit).
>> 
>> The commit message says "We'll want to fix it for real before the
>> release."  Is this patch for 2.5?  For what it's worth, it applies to
>> master.
>
> It's a bit late for 2.5 now, and I don't think we'll run into 2^32 uses
> of qnull(), so I can live with this waiting until 2.6 and additionally
> adding qemu-stable in cc.

If this patch was simple, I'd try to get it into 2.5.  But my questions
demonstrate it isn't simple, so we'll have to break our promise to "fix
it for real before the release".  *Sigh*

>>> This fixes things by documenting the internal contracts, and
>>> explaining why the internal function can return NULL and only
>>> the public facing interface needs to worry about qnull(),
>>> thus avoiding over-referencing the qnull_ global object.
>>>
>>> It does not, however, fix the stupidity of the stack mixing
>>> up two separate pieces of information; add a FIXME to explain
>>> that issue.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>>
>>> ---
>>> v6: no change
>>> ---
>>>  qapi/qmp-output-visitor.c       | 30 ++++++++++++++++++++++++++++--
>>>  tests/test-qmp-output-visitor.c |  2 ++
>>>  2 files changed, 30 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
>>> index e66ab3c..9d0f9d1 100644
>>> --- a/qapi/qmp-output-visitor.c
>>> +++ b/qapi/qmp-output-visitor.c
>>> @@ -29,6 +29,12 @@ typedef QTAILQ_HEAD(QStack, QStackEntry) QStack;
>>>  struct QmpOutputVisitor
>>>  {
>>>      Visitor visitor;
>>> +    /* FIXME: we are abusing stack to hold two separate pieces of
>>> +     * information: the current root object, and the stack of objects
>>> +     * still being built.  Worse, our behavior is inconsistent:
>>> +     * visiting two top-level scalars in a row discards the first in
>>> +     * favor of the second, but visiting two top-level objects in a
>>> +     * row tries to append the second object into the first.  */
>> 
>> Uhh, I'll simply take your word for it.
>
> I clean it up in 6/23, if reviewing that one helps understand the
> comment here.
>
>> 
>>>      QStack stack;
>>>  };
>>>
>>> @@ -41,10 +47,12 @@ static QmpOutputVisitor *to_qov(Visitor *v)
>>>      return container_of(v, QmpOutputVisitor, visitor);
>>>  }
>>>
>>> +/* Push @value onto the stack of current QObjects being built */
>>>  static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value)
>>>  {
>>>      QStackEntry *e = g_malloc0(sizeof(*e));
>>>
>>> +    assert(value);
>>>      e->value = value;
>> 
>> Keep in mind for later: we never push a null value on the stack, and
>> there is no other way to grow the stack.  Therefore, the stack holds
>> only non-null values.
>> 
>>>      if (qobject_type(e->value) == QTYPE_QLIST) {
>>>          e->is_list_head = true;
>>> @@ -52,16 +60,20 @@ static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value)
>>>      QTAILQ_INSERT_HEAD(&qov->stack, e, node);
>>>  }
>>>
>>> +/* Grab and remove the most recent QObject from the stack */
>>>  static QObject *qmp_output_pop(QmpOutputVisitor *qov)
>>>  {
>>>      QStackEntry *e = QTAILQ_FIRST(&qov->stack);
>>>      QObject *value;
>>> +
>>> +    assert(e);
>> 
>> I figure this assert the stack isn't empty.  Correct?
>
> Yes.
>
>> 
>>>      QTAILQ_REMOVE(&qov->stack, e, node);
>>>      value = e->value;
>>>      g_free(e);
>>>      return value;
>>>  }
>>>
>>> +/* Grab the root QObject, if any, in preparation to empty the stack */
>>>  static QObject *qmp_output_first(QmpOutputVisitor *qov)
>>>  {
>>>      QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack);
>>> @@ -72,24 +84,32 @@ static QObject *qmp_output_first(QmpOutputVisitor *qov)
>>        /*
>>         * FIXME Wrong, because qmp_output_get_qobject() will increment
>>         * the refcnt *again*.  We need to think through how visitors
>>>       * handle null.
>>>       */
>> 
>> Sure you don't want to drop the FIXME?
>
> Oops. I didn't rebase my patch quite correctly (when I first wrote it,
> commit 6c2f9a15 wasn't yet in final form).  Yes, the comment is dead now.
>
>> 
>>>      if (!e) {
>> 
>> I figure this means the stack is empty.  Now I wish I understood how we
>> use the stack.
>
> The stack is either empty (visit just started) or the head contains a
> root element (see qmp_output_add_obj calling qmp_output_push_obj).
> Additionally, if the head is a QDict or QList, then the 2nd through
> (N+1)th elements are the 1st through Nth incomplete containers still
> collecting members.

Explanation should go into a comment.  It's okay to add it only after
you rework the use of the stack if the current use defies description.

>>> -        return qnull();
>>> +        /* No root */
>>> +        return NULL;
>>>      }
>>> -
>>> +    assert(e->value);
>> 
>> Safe because the stack holds only non-null values.
>> 
>>>      return e->value;
>>>  }
>> 
>> We return null exactly when the stack is empty (whatever that may mean).

According to your explanation above, it means "nothing has been visited,
yet".

>>> +/* Grab the most recent QObject from the stack, which must exist */
>>>  static QObject *qmp_output_last(QmpOutputVisitor *qov)
>>>  {
>>>      QStackEntry *e = QTAILQ_FIRST(&qov->stack);
>>> +
>>> +    assert(e);
>>>      return e->value;
>>>  }
>> 
>> Can't return null, because the stack holds only non-null values.
>> 
>> assert(e && e->value) to match qmp_output_first()?
>
> Can't hurt.
>
>> 
>>>
>>> +/* Add @value to the current QObject being built.
>>> + * If the stack is visiting a dictionary or list, @value is now owned
>>> + * by that container. Otherwise, @value is now the root.  */
>>>  static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
>>>                                 QObject *value)
>>>  {
>>>      QObject *cur;
>>>
>>>      if (QTAILQ_EMPTY(&qov->stack)) {
>>> +        /* Stack was empty, track this object as root */
>>>          qmp_output_push_obj(qov, value);
>>>          return;
>>>      }
>>> @@ -98,13 +118,16 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
>>>
>>>      switch (qobject_type(cur)) {
>>>      case QTYPE_QDICT:
>>> +        assert(name);
>> 
>> Okay, but it really only converts one kind of crash into another one.
>> 
>>>          qdict_put_obj(qobject_to_qdict(cur), name, value);
>>>          break;
>>>      case QTYPE_QLIST:
>>>          qlist_append_obj(qobject_to_qlist(cur), value);
>>>          break;
>>>      default:
>>> +        /* The previous root was a scalar, replace it with a new root */
>>>          qobject_decref(qmp_output_pop(qov));
>>> +        assert(QTAILQ_EMPTY(&qov->stack));
>>>          qmp_output_push_obj(qov, value);
>>>          break;
>>>      }
>> 
>> Hand me the bucket.
>
> Yep, this was the gross abuse of the stack that the FIXME above tries to
> point out, and which 6/23 tries to clean up.
>
>> 
>>> @@ -205,11 +228,14 @@ static void qmp_output_type_any(Visitor *v, QObject **obj, const char *name,
>>>      qmp_output_add_obj(qov, name, *obj);
>>>  }
>>>
>>> +/* Finish building, and return the root object. Will not be NULL. */
>>>  QObject *qmp_output_get_qobject(QmpOutputVisitor *qov)
>>>  {
>>>      QObject *obj = qmp_output_first(qov);
>>>      if (obj) {
>> 
>> Non-empty stack.
>> 
>>>          qobject_incref(obj);
>>> +    } else {
>> 
>> Empty stack.
>> 
>>> +        obj = qnull();
>>>      }
>>>      return obj;
>>>  }
>> 
>> Thanks for your comments.  They help a lot with understanding the code,
>> and they also demonstrate that it's muddled crap %-}
>
> Indeed.
>
>> 
>> So, how does this contraption work?
>> 
>> A visitor cab encounter NULL only when it visits pointers (d'oh!).
>> Searching qapi-visit-core.c for **obj finds start_struct(),
>> start_implicit_struct(), type_str(), type_any().
>> 
>> As far as I can tell, start_implicit_struct() is for unboxed structs, so
>> NULL must not happen there.
>
> You are correct that start_implicit_struct is for unboxed substructs
> (the branch of a flat union).  And we should never get here with it
> being NULL (although until commit 20/23 of this series, it was because
> we are abusing the 'data' member of the union during visit_start_union()).

We should spell out "*obj isn't null" in visit_start_implicit_struct()'s
contract.  I'd assert it for good measure.

>>  This visitor doesn't implement it anyway.
>
> Well, not yet (but it IS on queue as a patch by Zoltán:
> http://repo.or.cz/qemu/ericb.git/commitdiff/9ee9f4ad)

That's opts-visitor.c.  This is qmp-output-visitor.c.

>> qmp_output_type_str() substitutes "" for NULL.  Ooookay.
>
> Not the smartest thing we've done, but don't know how hard it would be
> to fix (we DO have clients abusing this aspect of qapi that would have
> to be cleaned up to quit passing in incomplete qapi structs).

Disgusting :)

>                                                                Someday
> we may want to actually use qnull() instead of qstring("") (so that the
> JSON on the wire would read "name":null instead of "name":"").

QAPI's treatment of null is unsatisfactory.  Initially, QAPI ignored
null at the surface, and hacked around it below the surface (mapping C
NULL to "" is such a hack).  Since then, we added partial support for
null, and we still have all the hacks.  Needs a thorough rethink, but
not in this thread.

>                                                                 But if
> we make that change, we would still be inserting a non-NULL QObject onto
> the stack.

Yes.

We created qnull() instead of representing null as a null pointer
because we were afraid of violating unstated assumptions in crap code
like this.

>> qmp_output_type_any() crashes on NULL.  Can this happen?
>
> Again, if the QObject is trying to represent NULL, it does so with
> qnull() (a non-null QObject), so we should never pass in NULL.  We
> aren't using 'any' very heavily, so I doubt we have any broken clients.

A visit_type_FOO() visits a variable whose type is the C representation
of a FOOish QAPI type, and its obj parameter is a pointer to the C
representation.

With an input visitor, it writes to the visited variable, and with an
output visitor, it reads from it.

Example: visit_type_int64() visits an int64_t, and the obj parameter is
int64_t *.  Input visitors can write any int64_t value, and output
visitors can cope with any int64_t value.

Example: visit_type_any() visits a QObject *, and the obj parameter is
Object **.  Input visitors must not write a null pointer, and output
visitors need not cope with a null pointer.  In fact,
qmp_output_type_any() crashes on null pointer:

    #0  0x0000555555b44294 in qobject_type (obj=0x0)
        at /work/armbru/qemu/include/qapi/qmp/qobject.h:106
    #1  0x0000555555b4432d in qmp_output_push_obj (qov=0x555557495470, value=0x0)
        at /work/armbru/qemu/qapi/qmp-output-visitor.c:49
    #2  0x0000555555b444c8 in qmp_output_add_obj (qov=
        0x555557495470, name=0x555555b9e772 "unused", value=0x0)
        at /work/armbru/qemu/qapi/qmp-output-visitor.c:94
    #3  0x0000555555b448e1 in qmp_output_type_any (v=
        0x555557495470, obj=0x7fffffffc878, name=0x555555b9e772 "unused", errp=0x7fffffffc888) at /work/armbru/qemu/qapi/qmp-output-visitor.c:199

Feeding data that was built with an input visitor to an output visitor
is safe: no null pointers.

Feeding arbitrary data isn't: we crash on null.

For now, I think we should simply spell out the restrictions in
visit_type_any()'s contract.  We should revisit it (haha) when we
rethink null in QAPI.

>> qmp_output_start_struct() ignores its obj argument.  My best guess is
>> that this substitutes an empty dictionary for NULL.
>
> No one should ever be passing in NULL for a QDict, but yes, the code
> would create an empty QDict on output if that happened.  Maybe worth
> adding an assert; but not in this patch, because I don't know if we have
> broken clients.

qmp_output_start_struct() starts the visit of a variable whose type is
the C representation of a structured QAPI type, i.e. a pointer to the
generated C type.  The obj parameter is a pointer to this pointer, thus
void **.  Input visitors must not write a null pointer, and output
visitors need not cope with a null pointer.  qmp_output_start_struct()
chooses to cope: it substitutes an empty QDict.  Doesn't crash, but
violates the schema instead, hurrah.

Again, let's simply spell out the mess in the contract for now.

>> Okay, I give up: how do we get an empty stack and return qnull()?
>
> The only way to get an empty stack is to never visit anything in the
> first place.  See test-qmp-output-visitor.c:test_visitor_out_empty(),
> which creates a visitor with qmp_output_get_qobject() then immediately
> asks for the root object even though it didn't call any visit_type_FOO()
> to populate a root object.  And my recollection is that back in
> September you had an example use of qom-get that could trigger a failure
> early enough to run into the empty stack scenario, which is why you
> added qnull() in the first place.

Why would anyone create a QMP output visitor, visit nothing, then expect
the visitor to yield a value?  Smells funny, so let's find out more.

Running make check with suitably instrumented code finds exactly one
place: the getter for "spapr-dr-connector" property "fdt"
prop_get_fdt().  It gets exercised with QMP commands like

    { "execute": "qom-get",
      "arguments": {
          "path": "/machine/unattached/device[6]/dr-connector[1073741934]",
          "property": "fdt" } }

Let's find out how prop_get_fdt() works.

If sPAPRDRConnector member fdt is null, it returns without doing
anything.  The output visitor pulls a null out of its hat then.

Else, we iterate over the flattened device tree until the nesting level
goes back to zero:

* On FDT_BEGIN_NODE, visit_start_struct(), increment nesting level

* On FDT_END_NODE, visit_end_struct(), decrement nesting level

* On FDT_PROP, visit a list of uint8_t.  I guess the list could be
  empty.

The loop can produce a recursive structure

    T = list of uint8_t
      | struct where all members are T

Whether "just a list" can happen I can't say; the code lacks assertions
or comments.  Likewise, I can't say whether the structs can ever be
empty.

Taken together, prop_get_fdt() returns null | T.

Note that the value's structure is *dynamic*.  The best a statically
defined QAPI schema can do to describe it is 'any'.

qom-get is of course declared to return 'any', but so far I had thought
it was because declaring a strongly typed qom-get was impractical.  This
property pushes it from impractical to impossible.  Not sure that's a
good idea.  But I digress.

Is null the appropriate value of qom-get when there is no fdt?

It looks accidental:

* Initially, attempting to qmp_output_get_qobject() without having
  visited anything was *wrong* and crashed.

* Commit 1d10b44 (v2.1.0) papered over the crash by making it return
  NULL for empty stacks.  I'm not sure why Marcel did that back then.  I
  guess he somehow ran into the crash, but I can't see how.

  Returning NULL is problematic, because QObject * generally isn't
  supposed to be NULL.  Some places cope with it anyway.  The QMP core
  is such a place: it arbitrarily substitutes an empty dictionary for
  NULL.

* When this device was created in commit bbf5c87 (v2.4.0),
  prop_get_fdt() returning without doing anything should've made qom-get
  return the empty dictionary.  "Should've", because my attempts to
  create the device fail.  I tried with commit 6c2f9a15^ instead, and
  can see it return {} there.

* Commit 6c2f9a1 (not yet released) exchanges the paper we use to paper
  over the crash: return qnull() for empty stacks.  The commit message
  explains my reasons back then.  What we didn't see back then is that
  it changes this property's value.  I'm trying to get that change
  reverted for 2.5, by fixing the visitor abuse, i.e. do a proper empty
  struct visit.

Back to the QMP output visitor: is asking the QMP output visitor for the
visit's value when you haven't visited anything a sane thing to do?  I
doubt it.  I think we should go back to the initial behavior, and find
and fix the code that misuses visitors that way.

>>> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
>>> index 3078442..8e6fc33 100644
>>> --- a/tests/test-qmp-output-visitor.c
>>> +++ b/tests/test-qmp-output-visitor.c
>>> @@ -461,6 +461,8 @@ static void test_visitor_out_empty(TestOutputVisitorData *data,
>>>
>>>      arg = qmp_output_get_qobject(data->qov);
>>>      g_assert(qobject_type(arg) == QTYPE_QNULL);
>>> +    /* Check that qnull reference counting is sane */
>>> +    g_assert(arg->refcnt == 2);
>>>      qobject_decref(arg);
>>>  }
>> 

Re: [Qemu-devel] [PATCH v6 05/23] qmp: Fix reference-counting of qnull on empty output visit

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4256 bytes --]

On 12/03/2015 10:50 AM, Markus Armbruster wrote:

>>> So, how does this contraption work?
>>>
>>> A visitor cab encounter NULL only when it visits pointers (d'oh!).
>>> Searching qapi-visit-core.c for **obj finds start_struct(),
>>> start_implicit_struct(), type_str(), type_any().
>>>
>>> As far as I can tell, start_implicit_struct() is for unboxed structs, so
>>> NULL must not happen there.
>>
>> You are correct that start_implicit_struct is for unboxed substructs
>> (the branch of a flat union).  And we should never get here with it
>> being NULL (although until commit 20/23 of this series, it was because
>> we are abusing the 'data' member of the union during visit_start_union()).
> 
> We should spell out "*obj isn't null" in visit_start_implicit_struct()'s
> contract.  I'd assert it for good measure.

I've spent the better part of my afternoon playing with asserts to see
what works and what doesn't.  So far, I've learned:

We have input visitors that allow visit_start_struct(v, NULL, ...) (see
vl.c's use of opts-visitor).  This is done when we want to parse from
some other format into what qapi would accept by hierarchical
visit_type_FOO(), but don't actually have a qapi type to parse into.

We have output visitors that allow visit_start_struct(v, NULL, ...)
(most uses of visit_start outside of generated qapi-visit).  This is
done when we want to create certain output but don't have a qapi type,
so we instead are visiting types by hand.

We are inconsistent on how we pass things.  I think that ALL callers of
visit_start_struct() should pass one of two sets:
 valid type name, sizeof(that type), non-NULL obj (where input visitors
will do *obj = g_new0(size) if no other error is present; and where
output visitors should have an already-complete *obj)
or:
 NULL type name, size 0, NULL obj
But we aren't there yet - several clients have to be tweaked.

I'm also considering changing the signature of visit_start_struct() to
bundle the type name next to the type size (the current code inserts the
dictionary key name in between the two, making life a bit more awkward).

>>> qmp_output_type_any() crashes on NULL.  Can this happen?
>>
>> Again, if the QObject is trying to represent NULL, it does so with
>> qnull() (a non-null QObject), so we should never pass in NULL.  We
>> aren't using 'any' very heavily, so I doubt we have any broken clients.

And I argued in the other thread that the spapr prop_get_fdt() abuse
should probably explicitly use visit_type_any() with qnull() as its
subject, instead of relying on no visit at all, if it turns out that we
like 'null' for missing fdt as distinct from an fdt that is present but
empty.

> 
> A visit_type_FOO() visits a variable whose type is the C representation
> of a FOOish QAPI type, and its obj parameter is a pointer to the C
> representation.

Or NULL when there is no qapi type, and we are just using the
visit_type_* to parse or output things manually without the intermediate
qapi representation.


> For now, I think we should simply spell out the restrictions in
> visit_type_any()'s contract.  We should revisit it (haha) when we
> rethink null in QAPI.

Yes, this thread has given me ideas on how to improve my 7/23 patch
documentation of the visitor interfaces.

> Again, let's simply spell out the mess in the contract for now.

Or since we're (soon) at the start of 2.6 development, add some asserts,
with the ability to fix bugs or revert the asserts closer to that
release as needed.


> Taken together, prop_get_fdt() returns null | T.
> 
> Note that the value's structure is *dynamic*.  The best a statically
> defined QAPI schema can do to describe it is 'any'.

Or possibly an alternate.

> Back to the QMP output visitor: is asking the QMP output visitor for the
> visit's value when you haven't visited anything a sane thing to do?  I
> doubt it.  I think we should go back to the initial behavior, and find
> and fix the code that misuses visitors that way.

Sounds like adding assertions and fixing up fallout is worth attempting,
then.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v6 09/23] hmp: Improve use of qapi visitor

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1582 bytes --]

On 11/25/2015 05:23 PM, Eric Blake wrote:
> Cache the visitor in a local variable instead of repeatedly
> calling the accessor.  Pass NULL for the visit_start_struct()
> object (which matches the fact that we were already passing 0
> for the size argument, because we aren't using the visit to
> allocate a qapi struct).  Pass "object" for the struct name,
> for better error messages. Reflow the logic so that we don't
> have to undo an object_add().
> 
> A later patch will then split the error detection currently
> in visit_struct_end(), at which point we can again hoist the
> object_add() to occur before the label as one of the cleanups
> enabled by that split.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 

> 
> -    object_add(type, id, pdict, opts_get_visitor(ov), &err);
> -
>  out_end:
> -    visit_end_struct(opts_get_visitor(ov), &err_end);
> -    if (!err && err_end) {
> -        qmp_object_del(id, NULL);
> +    visit_end_struct(v, &err_end);
> +    if (!err && !err_end) {
> +        object_add(type, id, pdict, v, &err);
>      }

The attempt to avoid a qmp_object_del() cleanup on error was honorable,
but wrong.  Calling visit_end_struct() prior to passing 'v' to
object_add() means that object_add() is now visiting a different level
of {} than it was pre-patch, which showed up as a testsuite breakage
under 'make check-qtest'.  I'm reworking this patch (and the similar
change to vl.c in 10/23) for v7.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]