From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48518) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S9iLW-0002f7-1g for qemu-devel@nongnu.org; Mon, 19 Mar 2012 15:34:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S9iLT-0003S9-PD for qemu-devel@nongnu.org; Mon, 19 Mar 2012 15:34:21 -0400 Received: from mail-yx0-f173.google.com ([209.85.213.173]:49689) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S9iLT-0003R0-J4 for qemu-devel@nongnu.org; Mon, 19 Mar 2012 15:34:19 -0400 Received: by yenr5 with SMTP id r5so6709589yen.4 for ; Mon, 19 Mar 2012 12:34:18 -0700 (PDT) Message-ID: <4F678A37.1020101@codemonkey.ws> Date: Mon, 19 Mar 2012 14:34:15 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1332185368-18708-1-git-send-email-pbonzini@redhat.com> In-Reply-To: <1332185368-18708-1-git-send-email-pbonzini@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC/RFA PATCH] qapi: detect extra members inside structs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: lcapitulino@redhat.com, qemu-devel@nongnu.org, Michael Roth 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 > --- > 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",