Drop unused QIV_STACK_SIZE and unused qobject_input_start_struct()
parameter errp.
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Message-Id: <1488544368-30622-13-git-send-email-armbru@redhat.com>

The QObject input visitor has three error message formats:

* Parameter '%s' is missing
* "Invalid parameter type for '%s', expected: %s"
* "QMP input object member '%s' is unexpected"

The '%s' are member names (or "null", but I'll fix that later).

The last error message calls the thing "QMP input object member"
instead of "parameter".  Misleading when the visitor is used on
QObjects that don't come from QMP.  Change it to "Parameter '%s' is
unexpected".
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Message-Id: <1488544368-30622-12-git-send-email-armbru@redhat.com>

The QERR_ macros are leftovers from the days of "rich" error objects.

QERR_QMP_BAD_INPUT_OBJECT, QERR_QMP_BAD_INPUT_OBJECT_MEMBER,
QERR_QMP_EXTRA_MEMBER are used in just one place now, except for one
use that has crept into qobject-input-visitor.c.

Drop these macros, to make the (bad) error messages more visible.
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Message-Id: <1488544368-30622-10-git-send-email-armbru@redhat.com>

The QmpInputVisitor has no direct dependency on QMP. It is
valid to use it anywhere that one has a QObject. Rename it
to better reflect its functionality as a generic QObject
to QAPI converter.

The previous commit renamed the files, this one renames C identifiers.
Reviewed-by: NKevin Wolf <kwolf@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
Message-Id: <1475246744-29302-5-git-send-email-berrange@redhat.com>
Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
[Straightforwardly rebased, split into file and identifier rename]
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

The QMP visitors have no direct dependency on QMP. It is
valid to use them anywhere that one has a QObject. Rename them
to better reflect their functionality as a generic QObject
to QAPI converter.

This is the first of three parts: rename the files.  The next two
parts will rename C identifiers.  The split is necessary to make git
rename detection work.
Reviewed-by: NKevin Wolf <kwolf@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
[Split into file and identifier rename, two comments touched up]
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

The 'old' dispatch code returned a QERR_MISSING_PARAMETER for missing
parameters, but the qapi qmp_dispatch() code uses
QERR_INVALID_PARAMETER_TYPE.

Improve qapi code to return QERR_MISSING_PARAMETER where
appropriate.

Fix expected error message in iotests.
Signed-off-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: NAlberto Garcia <berto@igalia.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Message-Id: <20160930095948.3154-4-marcandre.lureau@redhat.com>
[Drop incorrect error_setg() from qmp_input_type_any() and
qmp_input_type_null()]
Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

This helps to figure out the expectations.
Signed-off-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Message-Id: <20160930095948.3154-3-marcandre.lureau@redhat.com>
Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

qiv->root should not be null, make that clearer with some assert.
Signed-off-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Message-Id: <20160930095948.3154-2-marcandre.lureau@redhat.com>
Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

Unlike the other visit methods, visit_type_any() and visit_type_null()
neglect to check whether qmp_input_get_object() succeeded.  They crash
when it fails.  Reproducer:

{ "execute": "qom-set",
  "arguments": { "path": "/machine", "property": "rtc-time" } }

Will crash with:

qapi/qapi-visit-core.c:277: visit_type_any: Assertion `!err != !*obj'
failed

Broken in commit 5c678ee8.  Fix by adding the missing error checks.
Signed-off-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Message-Id: <20160922203927.28241-3-marcandre.lureau@redhat.com>
Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
[Commit message rephrased]
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

This saves a lot of memory compared to a statically-sized array,
or at least 24kb could be considered a lot on an Atari ST.
It also makes the code more similar to QmpOutputVisitor.

This removes the limit on the depth of a QObject that can be processed
into a QAPI tree.  This is not a problem because QObjects can be
considered trusted; the text received on the QMP wire is untrusted
input, but the JSON parser already takes pains to limit the QObject tree
it creates.  We don't need the QMP input visitor to limit it again.
Signed-off-by: NPaolo Bonzini <pbonzini@redhat.com>
Message-Id: <1467906798-5312-3-git-send-email-pbonzini@redhat.com>
Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
[Commit message typo fixed]
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

Now that we have a polymorphic visit_free(), we no longer need
qmp_input_visitor_cleanup(); which in turn means we no longer
need to return a subtype from qmp_input_visitor_new() nor a
public upcast function.

Generated code changes to qmp-marshal.c look like:

|@@ -52,11 +52,10 @@ void qmp_marshal_add_fd(QDict *args, QOb
| {
|     Error *err = NULL;
|     AddfdInfo *retval;
|-    QmpInputVisitor *qiv = qmp_input_visitor_new(QOBJECT(args), true);
|     Visitor *v;
|     q_obj_add_fd_arg arg = {0};
|
|-    v = qmp_input_get_visitor(qiv);
|+    v = qmp_input_visitor_new(QOBJECT(args), true);
|     visit_start_struct(v, NULL, NULL, 0, &err);
|     if (err) {
|         goto out;
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1465490926-28625-8-git-send-email-eblake@redhat.com>
Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

Making each visitor provide its own (awkwardly-named) FOO_cleanup()
is unusual, when we can instead have a polymorphic visit_free()
interface.  Over the next few patches, we can use the polymorphic
functions to eliminate the need for a FOO_get_visitor() function
for accessing specific visitor functionality, once everything can
be accessed directly through the Visitor* interfaces.

The dealloc visitor is the first one converted to completely use
the new entry point, since qapi_dealloc_visitor_cleanup() was the
only reason that qapi_dealloc_get_visitor() existed, and only
generated and testsuite code was even using it.  With the new
visit_free() entry point in place, we no longer need to expose
the QapiDeallocVisitor subtype through qapi_dealloc_visitor_new(),
and can get by with less generated code, with diffs that look like:

| void qapi_free_ACPIOSTInfo(ACPIOSTInfo *obj)
| {
|-    QapiDeallocVisitor *qdv;
|     Visitor *v;
|
|     if (!obj) {
|         return;
|     }
|
|-    qdv = qapi_dealloc_visitor_new();
|-    v = qapi_dealloc_get_visitor(qdv);
|+    v = qapi_dealloc_visitor_new();
|     visit_type_ACPIOSTInfo(v, NULL, &obj, NULL);
|-    qapi_dealloc_visitor_cleanup(qdv);
|+    visit_free(v);
|}
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1465490926-28625-5-git-send-email-eblake@redhat.com>
Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

Rather than making the dealloc visitor track of stack of pointers
remembered during visit_start_* in order to free them during
visit_end_*, it's a lot easier to just make all callers pass the
same pointer to visit_end_*.  The generated code has access to the
same pointer, while all other users are doing virtual walks and
can pass NULL.  The dealloc visitor is then greatly simplified.

All three visit_end_*() functions intentionally take a void**,
even though the visit_start_*() functions differ between void**,
GenericList**, and GenericAlternate**.  This is done for several
reasons: when doing a virtual walk, passing NULL doesn't care
what the type is, but when doing a generated walk, we already
have to cast the caller's specific FOO* to call visit_start,
while using void** lets us use visit_end without a cast. Also,
an upcoming patch will add a clone visitor that wants to use
the same implementation for all three visit_end callbacks,
which is made easier if all three share the same signature.

For visitors with already track per-object state (the QMP visitors
via a stack, and the string visitors which do not allow nesting),
add an assertion that the caller is indeed passing the same
pointer to paired calls.
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1465490926-28625-4-git-send-email-eblake@redhat.com>
Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

The semantics of the list visit are somewhat baroque, with the
following pseudocode when FooList is used:

start()
for (prev = head; cur = next(prev); prev = &cur) {
    visit(&cur->value)
}

Note that these semantics (advance before visit) requires that
the first call to next() return the list head, while all other
calls return the next element of the list; that is, every visitor
implementation is required to track extra state to decide whether
to return the input as-is, or to advance.  It also requires an
argument of 'GenericList **' to next(), solely because the first
iteration might need to modify the caller's GenericList head, so
that all other calls have to do a layer of dereferencing.

Thankfully, we only have two uses of list visits in the entire
code base: one in spapr_drc (which completely avoids
visit_next_list(), feeding in integers from a different source
than uint8List), and one in qapi-visit.py.  That is, all other
list visitors are generated in qapi-visit.c, and share the same
paradigm based on a qapi FooList type, so we can refactor how
lists are laid out with minimal churn among clients.

We can greatly simplify things by hoisting the special case
into the start() routine, and flipping the order in the loop
to visit before advance:

start(head)
for (tail = *head; tail; tail = next(tail)) {
    visit(&tail->value)
}

With the simpler semantics, visitors have less state to track,
the argument to next() is reduced to 'GenericList *', and it
also becomes obvious whether an input visitor is allocating a
FooList during visit_start_list() (rather than the old way of
not knowing if an allocation happened until the first
visit_next_list()).  As a minor drawback, we now allocate in
two functions instead of one, and have to pass the size to
both functions (unless we were to tweak the input visitors to
cache the size to start_list for reuse during next_list, but
that defeats the goal of less visitor state).

The signature of visit_start_list() is chosen to match
visit_start_struct(), with the new parameters after 'name'.

The spapr_drc case is a virtual visit, done by passing NULL for
list, similarly to how NULL is passed to visit_start_struct()
when a qapi type is not used in those visits.  It was easy to
provide these semantics for qmp-output and dealloc visitors,
and a bit harder for qmp-input (several prerequisite patches
refactored things to make this patch straightforward).  But it
turned out that the string and opts visitors munge enough other
state during visit_next_list() to make it easier to just
document and require a GenericList visit for now; an assertion
will remind us to adjust things if we need the semantics in the
future.

Several pre-requisite cleanup patches made the reshuffling of
the various visitors easier; particularly the qmp input visitor.
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1461879932-9020-24-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

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.  So, split out the error checking
portion (basically, input visitors checking for unvisited keys) into
a new function visit_check_struct(), which can be safely skipped if
any earlier errors are encountered, and leave the cleanup portion
(which never fails, but must be called unconditionally if
visit_start_struct() succeeded) in visit_end_struct().

Generated code in qapi-visit.c has diffs resembling:

|@@ -59,10 +59,12 @@ void visit_type_ACPIOSTInfo(Visitor *v,
|         goto out_obj;
|     }
|     visit_type_ACPIOSTInfo_members(v, obj, &err);
|-    error_propagate(errp, err);
|-    err = NULL;
|+    if (err) {
|+        goto out_obj;
|+    }
|+    visit_check_struct(v, &err);
| out_obj:
|-    visit_end_struct(v, &err);
|+    visit_end_struct(v);
| out:

and in qapi-event.c:

@@ -47,7 +47,10 @@ void qapi_event_send_acpi_device_ost(ACP
|         goto out;
|     }
|     visit_type_q_obj_ACPI_DEVICE_OST_arg_members(v, &param, &err);
|-    visit_end_struct(v, err ? NULL : &err);
|+    if (!err) {
|+        visit_check_struct(v, &err);
|+    }
|+    visit_end_struct(v);
|     if (err) {
|         goto out;
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1461879932-9020-20-git-send-email-eblake@redhat.com>
[Conflict with a doc fixup resolved]
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

Implement the new type_null() callback for the qmp input and
output visitors. While we don't yet have a use for this in QAPI
input (the generator will need some tweaks first), some
potential usages have already been discussed on the list.
Meanwhile, the output visitor could already output explicit null
via type_any, but this gives us finer control.

At any rate, it's easy to test that we can round-trip an explicit
null through manual use of visit_type_null() wrapped by a virtual
visit_start_struct() walk, even if we can't do the visit in a
QAPI type.  Repurpose the test_visitor_out_empty test,
particularly since a future patch will tighten semantics to
forbid use of qmp_output_get_qobject() without at least one
intervening visit_type_*.
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1461879932-9020-16-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

Right now, qmp-output-visitor happens to produce a QNull result
if nothing is actually visited between the creation of the visitor
and the request for the resulting QObject.  A stronger protocol
would require that a QMP output visit MUST visit something.  But
to still be able to produce a JSON 'null' output, we need a new
visitor function that states our intentions.  Yes, we could say
that such a visit must go through visit_type_any(), but that
feels clunky.

So this patch introduces the new visit_type_null() interface and
its no-op interface in the dealloc visitor, and stubs in the
qmp visitors (the next patch will finish the implementation).
For the visitors that will not implement the callback, document
the situation. The code in qapi-visit-core unconditionally
dereferences the callback pointer, so that a segfault will inform
a developer if they need to implement the callback for their
choice of visitor.

Note that JSON has a primitive null type, with the single value
null; likewise with the QNull type for QObject; but for QAPI,
we just have the 'null' value without a null type.  We may
eventually want to add more support in QAPI for null (most likely,
we'd use it via an alternate type that permits 'null' or an
object); but we'll create that usage when we need it.
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1461879932-9020-15-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

In the QMP input visitor, visiting a list traverses two objects:
the QAPI GenericList of the caller (which gets advanced in
visit_next_list() regardless of this patch), and the QList input
that we are converting to QAPI.  For consistency with QDict
visits, we want to consume elements from the input QList during
the visit_type_FOO() for the list element; that is, we want ALL
the code for consuming an input to live in qmp_input_get_object(),
rather than having it split according to whether we are visiting
a dict or a list.  Making qmp_input_get_object() the common point
of consumption will make it easier for a later patch to refactor
visit_start_list() to cover the GenericList * head of a QAPI list,
and in turn will get rid of the 'first' flag (which lived in
qmp_input_next_list() pre-patch, and is hoisted to StackObject
by this patch).

This patch is therefore altering the post-condition use of 'entry',
while keeping what gets visited unchanged, from:

        start_list next_list type_ELT ... next_list type_ELT next_list end_list
 visits                      1st elt                last elt
 entry  NULL       1st elt   1st elt      last elt  last elt NULL      gone

where type_ELT() returns (entry ? entry : 1st elt) and next_list() steps
entry

to this usage:

        start_list next_list type_ELT ... next_list type_ELT next_list end_list
 visits                      1st elt                last elt
 entry  1st elt    1nd elt   2nd elt      last elt  NULL     NULL      gone

where type_ELT() steps entry and returns the old entry, and next_list()
leaves entry alone.
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1461879932-9020-12-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

Don't embed the root of the visit into the stack of current
containers being visited.  That way, we no longer get confused
on whether the first visit of a dictionary is to the dictionary
itself or to one of the members of the dictionary, based on
whether the caller passed name=NULL; and makes the QMP Input
visitor like other visitors where the value of 'name' is now
ignored on the root visit.  (We may someday want to revisit
the rules on what 'name' should be on a top-level visit,
rather than just ignoring it; but that would be the topic of
another patch).

An audit of all qmp_input_visitor_new() call sites shows that
there were only two places where callers had previously been
visiting to a QDict with a non-NULL name to bypass a call to
visit_start_struct(), and those were fixed in prior patches.
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1461879932-9020-11-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

Commit e8316d7e mistakenly passed consume=true within
qmp_input_optional() when checking if an optional member was
present, but the mistake was silently ignored since the code
happily let us extract a member more than once.  Fix
qmp_input_optional() to not consume anything, then tighten up
the input visitor to ensure that a member is consumed exactly
once (all generated code follows this pattern; and the new
assert will catch any hand-written code that tries to visit
the same key more than once).
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1461879932-9020-8-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

Rather than having two separate ways to create a QMP input
visitor, where the safer approach has the more verbose name,
it is better to consolidate things into a single function
where the caller must explicitly choose whether to be strict
or to ignore excess input.  This patch is the strictly
mechanical conversion; the next patch will then audit which
uses can be made stricter.
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1461879932-9020-6-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

Management of the top of stack was a bit verbose; creating a
temporary variable and adding some comments makes the existing
code more legible before the next few patches improve things.
No semantic changes other than asserting that we are always
visiting a QObject, and not a NULL value.  In particular, the
check for 'name && qobject_type(qobj) == QTYPE_QDICT)' is a
bit overkill (a dict visit should always have a name); a later
patch revisits that, while this patch is only changing one
layer of indentation due to dropping 'if (qobj)'.
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1461879932-9020-5-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

Our existing input visitors were not very consistent on errors in a
function taking 'TYPE **obj'.  These are start_struct(),
start_alternate(), type_str(), and type_any().  next_list() is
similar, but can't fail (see commit 08f9541d).  While all of them set
'*obj' to allocated storage on success, it was not obvious whether
'*obj' was guaranteed safe on failure, or whether it was left
uninitialized.  But a future patch wants to guarantee that
visit_type_FOO() does not leak a partially-constructed obj back to
the caller; it is easier to implement this if we can reliably state
that input visitors assign '*obj' regardless of success or failure,
and that on failure *obj is NULL.  Add assertions to enforce
consistency in the final setting of err vs. *obj.

The opts-visitor start_struct() doesn't set an error, but it
also was doing a weird check for 0 size; all callers pass in
non-zero size if obj is non-NULL.

The testsuite has at least one spot where we no longer need
to pre-initialize a variable prior to a visit; valgrind confirms
that the test is still fine with the cleanup.

A later patch will document the design constraint implemented
here.
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1461879932-9020-3-git-send-email-eblake@redhat.com>
[visit_start_alternate()'s assertion tightened, commit message tweaked]
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

We have three classes of QAPI visitors: input, output, and dealloc.
Currently, all implementations of these visitors have one thing in
common based on their visitor type: the implementation used for the
visit_type_enum() callback.  But since we plan to add more such
common behavior, in relation to documenting and further refining
the semantics, it makes more sense to have the visitor
implementations advertise which class they belong to, so the common
qapi-visit-core code can use that information in multiple places.

A later patch will better document the types of visitors directly
in visitor.h.

For this patch, knowing the class of a visitor implementation lets
us make input_type_enum() and output_type_enum() become static
functions, by replacing the callback function Visitor.type_enum()
with the simpler enum member Visitor.type.  Share a common
assertion in qapi-visit-core as part of the refactoring.

Move comments in opts-visitor.c to match the refactored layout.
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1461879932-9020-2-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

Commit 57cb38b3 included qapi/error.h into qemu/osdep.h to get the
Error typedef.  Since then, we've moved to include qemu/osdep.h
everywhere.  Its file comment explains: "To avoid getting into
possible circular include dependencies, this file should not include
any other QEMU headers, with the exceptions of config-host.h,
compiler.h, os-posix.h and os-win32.h, all of which are doing a
similar job to this file and are under similar constraints."
qapi/error.h doesn't do a similar job, and it doesn't adhere to
similar constraints: it includes qapi-types.h.  That's in excess of
100KiB of crap most .c files don't actually need.

Add the typedef to qemu/typedefs.h, and include that instead of
qapi/error.h.  Include qapi/error.h in .c files that need it and don't
get it now.  Include qapi-types.h in qom/object.h for uint16List.

Update scripts/clean-includes accordingly.  Update it further to match
reality: replace config.h by config-target.h, add sysemu/os-posix.h,
sysemu/os-win32.h.  Update the list of includes in the qemu/osdep.h
comment quoted above similarly.

This reduces the number of objects depending on qapi/error.h from "all
of them" to less than a third.  Unfortunately, the number depending on
qapi-types.h shrinks only a little.  More work is needed for that one.
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
[Fix compilation without the spice devel packages. - Paolo]
Signed-off-by: NPaolo Bonzini <pbonzini@redhat.com>

After recent changes, the only remaining use of
visit_start_implicit_struct() is for allocating the space needed
when visiting an alternate.  Since the term 'implicit struct' is
hard to explain, rename the function to its current usage.  While
at it, we can merge the functionality of visit_get_next_type()
into the same function, making it more like visit_start_struct().

Generated code is now slightly smaller:

| {
|     Error *err = NULL;
|
|-    visit_start_implicit_struct(v, (void**) obj, sizeof(BlockdevRef), &err);
|+    visit_start_alternate(v, name, (GenericAlternate **)obj, sizeof(**obj),
|+                          true, &err);
|     if (err) {
|         goto out;
|     }
|-    visit_get_next_type(v, name, &(*obj)->type, true, &err);
|-    if (err) {
|-        goto out_obj;
|-    }
|     switch ((*obj)->type) {
|     case QTYPE_QDICT:
|         visit_start_struct(v, name, NULL, 0, &err);
...
|     }
|-out_obj:
|-    visit_end_implicit_struct(v);
|+    visit_end_alternate(v);
| out:
|     error_propagate(errp, err);
| }
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1455778109-6278-16-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

By sticking the next pointer first, we don't need a union with
64-bit padding for smaller types.  On 32-bit platforms, this
can reduce the size of uint8List from 16 bytes (or 12, depending
on whether 64-bit ints can tolerate 4-byte alignment) down to 8.
It has no effect on 64-bit platforms (where alignment still
dictates a 16-byte struct); but fewer anonymous unions is still
a win in my book.

It requires visit_next_list() to gain a size parameter, to know
what size element to allocate; comparable to the size parameter
of visit_start_struct().

I debated about going one step further, to allow for fewer casts,
by doing:
    typedef GenericList GenericList;
    struct GenericList {
        GenericList *next;
    };
    struct FooList {
        GenericList base;
        Foo *value;
    };
so that you convert to 'GenericList *' by '&foolist->base', and
back by 'container_of(generic, GenericList, base)' (as opposed to
the existing '(GenericList *)foolist' and '(FooList *)generic').
But doing that would require hoisting the declaration of
GenericList prior to inclusion of qapi-types.h, rather than its
current spot in visitor.h; it also makes iteration a bit more
verbose through 'foolist->base.next' instead of 'foolist->next'.

Note that for lists of objects, the 'value' payload is still
hidden behind a boxed pointer.  Someday, it would be nice to do:

struct FooList {
    FooList *next;
    Foo value;
};

for one less level of malloc for each list element.  This patch
is a step in that direction (now that 'next' is no longer at a
fixed non-zero offset within the struct, we can store more than
just a pointer's-worth of data as the value payload), but the
actual conversion would be a task for another series, as it will
touch a lot of code.
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1455778109-6278-10-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

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 an arbitrary entry, we can get that from a single-use
iterator, without needing a tautological callback function.

Technically, our cast of &(GQueue *) to (void **) is not strict
C (while void * must be able to hold all other pointers, nothing
says a void ** has to be the same width or representation as a
GQueue **).  The kosher way to write it would be the verbose:

    void *tmp;
    GQueue *any;
    if (g_hash_table_iter_next(&iter, NULL, &tmp)) {
        any = tmp;

But our code base (not to mention glib itself) already has other
cases of assuming that ALL pointers have the same width and
representation, where a compiler would have to go out of its way
to mis-compile our borderline behavior.
Suggested-by: NMarkus Armbruster <armbru@redhat.com>
Signed-off-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <1455778109-6278-2-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

No backend was setting an error when ending the visit of a list or
implicit struct, or when moving to the next list node.  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: NEric Blake <eblake@redhat.com>
Message-Id: <1454075341-13658-24-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

The only way that qmp_input_pop() will set errp is if a dictionary
was the most recent thing pushed.  Since we don't have any
push(struct)/pop(list) or push(list)/pop(struct) mismatches (such
a mismatch is a programming bug), we therefore cannot set errp
inside qmp_input_end_list().  Make this obvious by
using &error_abort.  A later patch will then remove the errp
parameter of qmp_input_pop(), but that will first require the
larger task of splitting visit_end_struct().
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1454075341-13658-23-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

visit_start_struct() and visit_type_enum() had a 'kind' argument
that was usually set to either the stringized version of the
corresponding qapi type name, or to NULL (although some clients
didn't even get that right).  But nothing ever used the argument.
It's even hard to argue that it would be useful in a debugger,
as a stack backtrace also tells which type is being visited.

Therefore, drop the 'kind' argument as dead.
Signed-off-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <1454075341-13658-22-git-send-email-eblake@redhat.com>
[Harmless rebase mistake cleaned up]
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

As explained in the previous patches, matching argument order of
'name, &value' to JSON's "name":value makes sense.  However,
while the last two patches were easy with Coccinelle, I ended up
doing this one all by hand.  Now all the visitor callbacks match
the main interface.

The compiler is able to enforce that all clients match the changed
interface in visitor-impl.h, even where two pointers are being
swapped, because only one of the two pointers is const (if that
were not the case, then C's looseness on treating 'char *' like
'void *' would have made review a bit harder).
Signed-off-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <1454075341-13658-21-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

Our qapi visitor contract supports multiple integer visitors,
but left the type_uint64 visitor as optional (falling back on
type_int64); which in turn can lead to awkward behavior with
numbers larger than INT64_MAX (the user has to be aware of
twos complement, and deal with negatives).

This patch does not address the disparity in handling large
values as negatives.  It merely moves the fallback from uint64
to int64 from the visitor core to the visitors, where the issue
can actually be fixed, by implementing the missing type_uint64()
callbacks on top of the respective type_int64() callbacks, and
with a FIXME comment explaining why that's wrong.

With that done, we now have a type_uint64() callback in every
driver, so we can make it mandatory from the core.  And although
the type_int64() callback can cover the entire valid range of
type_uint{8,16,32} on valid user input, using type_uint64() to
avoid mixed signedness makes more sense.
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1454075341-13658-15-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

The qapi builtin type 'int' is basically shorthand for the type
'int64'.  In fact, since no visitor was providing the optional
type_int64() callback, visit_type_int64() was just always falling
back to type_int(), cementing the equivalence between the types.

However, some visitors are providing a type_uint64() callback.
For purposes of code consistency, it is nicer if all visitors
use the paired type_int64/type_uint64 names rather than the
mismatched type_int/type_uint64.  So this patch just renames
the signed int callbacks in place, dropping the type_int()
callback as redundant, and a later patch will focus on the
unsigned int callbacks.

Add some FIXMEs to questionable reuse of errp in code touched
by the rename, while at it (the reuse works as long as the
callbacks don't modify value when setting an error, but it's not
a good example to set) - a later patch will then fix those.

No change in functionality here, although further cleanups are
in the pipeline.
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1454075341-13658-14-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

Clean up includes so that osdep.h is included first and headers
which it implies are not included manually.

This commit was created with scripts/clean-includes.
Signed-off-by: NPeter Maydell <peter.maydell@linaro.org>
Reviewed-by: NEric Blake <eblake@redhat.com>
Message-id: 1454089805-5470-8-git-send-email-peter.maydell@linaro.org

None of the visitor callbacks would set an error when testing
if an optional field was present; make this part of the interface
contract by eliminating the errp argument.

The resulting generated code has a nice diff:

|-    visit_optional(v, &has_fdset_id, "fdset-id", &err);
|-    if (err) {
|-        goto out;
|-    }
|+    visit_optional(v, &has_fdset_id, "fdset-id");
|     if (has_fdset_id) {
|         visit_type_int(v, &fdset_id, "fdset-id", &err);
|         if (err) {
|             goto out;
|         }
|     }
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1449033659-25497-9-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

The QMP input visitor allows integral values to be assigned by
promotion to a QTYPE_QFLOAT.  However, when parsing an alternate,
we did not take this into account, such that an alternate that
accepts 'number' and some other type, but not 'int', would reject
integral values.

With this patch, we now have the following desirable table:

    alternate has      case selected for
    'int'  'number'    QTYPE_QINT  QTYPE_QFLOAT
      no        no     error       error
      no       yes     'number'    'number'
     yes        no     'int'       error
     yes       yes     'int'       'number'

While it is unlikely that we will ever use 'number' in an
alternate other than in the testsuite, it never hurts to be
more precise in what we allow.
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1449033659-25497-8-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

Previously, working with alternates required two lookup arrays
and some indirection: for type Foo, we created Foo_qtypes[]
which maps each qtype to a value of the generated FooKind enum,
then look up that value in FooKind_lookup[] like we do for other
union types.

This has a couple of subtle bugs.  First, the generator was
creating a call with a parameter '(int *) &(*obj)->type' where
type is an enum type; this is unsafe if the compiler chooses
to store the enum type in a different size than int, where
assigning through the wrong size pointer can corrupt data or
cause a SIGBUS.

Related bug, not not fixed in this patch: qapi-visit.py's
gen_visit_enum() generates a cast of its enum * argument to
int *. Marked FIXME.

Second, since the values of the FooKind enum start at zero, all
entries of the Foo_qtypes[] array that were not explicitly
initialized will map to the same branch of the union as the
first member of the alternate, rather than triggering a desired
failure in visit_get_next_type().  Fortunately, the bug seldom
bites; the very next thing the input visitor does is try to
parse the incoming JSON with the wrong parser, which normally
fails; the output visitor is not used with a C struct in that
state, and the dealloc visitor has nothing to clean up (so
there is no leak).

However, the second bug IS observable in one case: parsing an
integer causes unusual behavior in an alternate that contains
at least a 'number' member but no 'int' member, because the
'number' parser accepts QTYPE_QINT in addition to the expected
QTYPE_QFLOAT (that is, since 'int' is not a member, the type
QTYPE_QINT accidentally maps to FooKind 0; if this enum value
is the 'number' branch the integer parses successfully, but if
the 'number' branch is not first, some other branch tries to
parse the integer and rejects it).  A later patch will worry
about fixing alternates to always parse all inputs that a
non-alternate 'number' would accept, for now this is still
marked FIXME in the updated test-qmp-input-visitor.c, to
merely point out that new undesired behavior of 'ans' matches
the existing undesired behavior of 'asn'.

This patch fixes the default-initialization bug by deleting the
indirection, and modifying get_next_type() to directly assign a
QTypeCode parameter.  This in turn fixes the type-casting bug,
as we are no longer casting a pointer to enum to a questionable
size. There is no longer a need to generate an implicit FooKind
enum associated with the alternate type (since the QMP wire
format never uses the stringized counterparts of the C union
member names).  Since the updated visit_get_next_type() does not
know which qtypes are expected, the generated visitor is
modified to generate an error statement if an unexpected type is
encountered.

Callers now have to know the QTYPE_* mapping when looking at the
discriminator; but so far, only the testsuite was even using the
C struct of an alternate types.  I considered the possibility of
keeping the internal enum FooKind, but initialized differently
than most generated arrays, as in:
  typedef enum FooKind {
      FOO_KIND_A = QTYPE_QDICT,
      FOO_KIND_B = QTYPE_QINT,
  } FooKind;
to create nicer aliases for knowing when to use foo->a or foo->b
when inspecting foo->type; but it turned out to add too much
complexity, especially without a client.

There is a user-visible side effect to this change, but I
consider it to be an improvement. Previously,
the invalid QMP command:
  {"execute":"blockdev-add", "arguments":{"options":
    {"driver":"raw", "id":"a", "file":true}}}
failed with:
  {"error": {"class": "GenericError",
    "desc": "Invalid parameter type for 'file', expected: QDict"}}
(visit_get_next_type() succeeded, and the error comes from the
visit_type_BlockdevOptions() expecting {}; there is no mention of
the fact that a string would also work).  Now it fails with:
  {"error": {"class": "GenericError",
    "desc": "Invalid parameter type for 'file', expected: BlockdevRef"}}
(the error when the next type doesn't match any expected types for
the overall alternate).
Signed-off-by: NEric Blake <eblake@redhat.com>
Message-Id: <1449033659-25497-5-git-send-email-eblake@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

qobject_to_qstring() crashes on null, which is a trap for the unwary.
Return null instead, and simplify a few callers.
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Message-Id: <1444918537-18107-7-git-send-email-armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NLuiz Capitulino <lcapitulino@redhat.com>

qobject_to_qfloat() and qobject_to_qint() crash on null, which is a
trap for the unwary.  Return null instead, and simplify a few callers.
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Message-Id: <1444918537-18107-5-git-send-email-armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NLuiz Capitulino <lcapitulino@redhat.com>