Re: [Qemu-devel] [RFC/RFA PATCH] qapi: detect extra members inside structs

[Anthony Liguori <anthony@codemonkey.ws>]

On 03/19/2012 02:29 PM, 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<pbonzini@redhat.com>
> ---
> 	Is this acceptable or just wrong?

This is a feature.  The idea is that with QMP, old clients just ignore extra 
members in a structure.  I've never felt that comfortable with this as a 
semantic but this is how QMP was designed.

If you don't allow this semantic, then it's impossible to ever add a field to an 
existing type as that would break backwards compatibility.

Regards,

Anthony Liguori

>
> 	Other small problems I noticed in running the testsuite:
>          why is qemu-ga$(EXESUF) in tests/Makefile and test-qmp-commands
>          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",