On Mon, Mar 19, 2012 at 08:29:28PM +0100, Paolo Bonzini wrote:
> I noticed that the QMP input visitor does not detect extra members inside
> structs. The outermost arguments struct is handled by the QMP type
> checker, but the nested ones go undetected. That could be a problem
> for complex commands such as "transaction".
>
> This patch adds such detection to the QMP input visitor.
>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
> Is this acceptable or just wrong?
>
> Other small problems I noticed in running the testsuite:
> why is qemu-ga$(EXESUF) in tests/Makefile and test-qmp-commands
qemu-ga looks like an inadvertant copy/paste. and yah, test-qmp-commands
should be part of make check, might've been left out initially because it
barfs out debug statements. Just sent some patches.
> not run by "make check"?
>
> qapi/qmp-input-visitor.c | 19 ++++++++++++++++--
> qerror.h | 3 +++
> test-qmp-input-visitor.c | 19 +++++++++++++++++++
> 3 files changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index e6b6152..416ab90 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -23,7 +23,8 @@
> typedef struct StackObject
> {
> const QObject *obj;
> - const QListEntry *entry;
> + const QListEntry *entry;
> + int left;
> } StackObject;
>
> struct QmpInputVisitor
> @@ -31,6 +32,7 @@ struct QmpInputVisitor
> Visitor visitor;
> QObject *obj;
> StackObject stack[QIV_STACK_SIZE];
> + int left;
> int nb_stack;
> };
>
> @@ -52,8 +54,13 @@ static const QObject *qmp_input_get_object(QmpInputVisitor
> *qiv,
>
> if (qobj) {
> if (name && qobject_type(qobj) == QTYPE_QDICT) {
> - return qdict_get(qobject_to_qdict(qobj), name);
> + qobj = qdict_get(qobject_to_qdict(qobj), name);
> + if (qobj) {
> + assert(qiv->left > 0);
> + qiv->left--;
> + }
> } else if (qiv->nb_stack > 0 && qobject_type(qobj) == QTYPE_QLIST) {
> + assert(qiv->left == -1);
> return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
> }
> }
> @@ -64,10 +70,12 @@ static const QObject
> *qmp_input_get_object(QmpInputVisitor *qiv,
> static void qmp_input_push(QmpInputVisitor *qiv, const QObject *obj, Error
> **errp)
> {
> qiv->stack[qiv->nb_stack].obj = obj;
> + qiv->stack[qiv->nb_stack].left = qiv->left;
> if (qobject_type(obj) == QTYPE_QLIST) {
> qiv->stack[qiv->nb_stack].entry = qlist_first(qobject_to_qlist(obj));
> }
> qiv->nb_stack++;
> + qiv->left = -1;
>
> if (qiv->nb_stack >= QIV_STACK_SIZE) {
> error_set(errp, QERR_BUFFER_OVERRUN);
> @@ -77,7 +85,12 @@ static void qmp_input_push(QmpInputVisitor *qiv, const
> QObject *obj, Error **err
>
> static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
> {
> + if (qiv->left != -1 && qiv->left != 0) {
> + error_set(errp, QERR_EXTRA_MEMBER);
> + return;
> + }
> qiv->nb_stack--;
> + qiv->left = qiv->stack[qiv->nb_stack].left;
> if (qiv->nb_stack < 0) {
> error_set(errp, QERR_BUFFER_OVERRUN);
> return;
> @@ -97,6 +110,7 @@ static void qmp_input_start_struct(Visitor *v, void **obj,
> const char *kind,
> }
>
> qmp_input_push(qiv, qobj, errp);
> + qiv->left = qdict_size(qobject_to_qdict(qobj));
> if (error_is_set(errp)) {
> return;
> }
> diff --git a/qerror.h b/qerror.h
> index e26c635..520cdab 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -127,6 +127,9 @@ QError *qobject_to_qerror(const QObject *obj);
> #define QERR_DUPLICATE_ID \
> "{ 'class': 'DuplicateId', 'data': { 'id': %s, 'object': %s } }"
>
> +#define QERR_EXTRA_MEMBER \
> + "{ 'class': 'ExtraInputObjectMember', 'data': {} }"
> +
> #define QERR_FD_NOT_FOUND \
> "{ 'class': 'FdNotFound', 'data': { 'name': %s } }"
>
> diff --git a/test-qmp-input-visitor.c b/test-qmp-input-visitor.c
> index 1996e49..1783759 100644
> --- a/test-qmp-input-visitor.c
> +++ b/test-qmp-input-visitor.c
> @@ -161,6 +161,23 @@ static void visit_type_TestStruct(Visitor *v, TestStruct
> **obj,
> visit_end_struct(v, errp);
> }
>
> +static void test_visitor_in_struct_extra(TestInputVisitorData *data,
> + const void *unused)
> +{
> + TestStruct *p = NULL;
> + Error *errp = NULL;
> + Visitor *v;
> +
> + v = visitor_input_test_init(data, "{ 'integer': -42, 'boolean': true,
> 'string': 'foo', 'extra' : [ 123, 456, 'def' ] }");
> +
> + visit_type_TestStruct(v, &p, NULL, &errp);
> + g_assert(error_is_set(&errp));
> + if (p) {
> + g_free(p->string);
> + g_free(p);
> + }
> +}
> +
> static void test_visitor_in_struct(TestInputVisitorData *data,
> const void *unused)
> {
> @@ -278,6 +295,8 @@ int main(int argc, char **argv)
> &in_visitor_data, test_visitor_in_struct);
> input_visitor_test_add("/visitor/input/struct-nested",
> &in_visitor_data, test_visitor_in_struct_nested);
> + input_visitor_test_add("/visitor/input/struct-extra",
> + &in_visitor_data, test_visitor_in_struct_extra);
> input_visitor_test_add("/visitor/input/list",
> &in_visitor_data, test_visitor_in_list);
> input_visitor_test_add("/visitor/input/union",
> --
> 1.7.7.6
>
>
