From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:37113) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QUi3r-0006XN-Ho for qemu-devel@nongnu.org; Thu, 09 Jun 2011 12:26:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QUi3p-0003MA-AR for qemu-devel@nongnu.org; Thu, 09 Jun 2011 12:26:23 -0400 Received: from e31.co.us.ibm.com ([32.97.110.149]:38716) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QUi3o-0003M3-MT for qemu-devel@nongnu.org; Thu, 09 Jun 2011 12:26:21 -0400 Received: from d03relay02.boulder.ibm.com (d03relay02.boulder.ibm.com [9.17.195.227]) by e31.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id p59G9aqR014526 for ; Thu, 9 Jun 2011 10:09:36 -0600 Received: from d03av06.boulder.ibm.com (d03av06.boulder.ibm.com [9.17.195.245]) by d03relay02.boulder.ibm.com (8.13.8/8.13.8/NCO v9.1) with ESMTP id p59GQ2Y9126942 for ; Thu, 9 Jun 2011 10:26:03 -0600 Received: from d03av06.boulder.ibm.com (loopback [127.0.0.1]) by d03av06.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p59GVHWT023533 for ; Thu, 9 Jun 2011 10:31:17 -0600 Message-ID: <4DF0F418.4030708@linux.vnet.ibm.com> Date: Thu, 09 Jun 2011 11:26:00 -0500 From: Michael Roth MIME-Version: 1.0 References: <1307140399-9023-1-git-send-email-mdroth@linux.vnet.ibm.com> <1307140399-9023-11-git-send-email-mdroth@linux.vnet.ibm.com> <20110609123009.1082ecd8@doriath> <4DF0E9C1.6040406@linux.vnet.ibm.com> <20110609125510.500ce135@doriath> In-Reply-To: <20110609125510.500ce135@doriath> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2][ 10/21] qapi: add QMP input visiter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: aliguori@linux.vnet.ibm.com, agl@linux.vnet.ibm.com, qemu-devel@nongnu.org, Jes.Sorensen@redhat.com On 06/09/2011 10:55 AM, Luiz Capitulino wrote: > On Thu, 09 Jun 2011 10:41:53 -0500 > Michael Roth wrote: > >> On 06/09/2011 10:30 AM, Luiz Capitulino wrote: >>> On Fri, 3 Jun 2011 17:33:08 -0500 >>> Michael Roth wrote: >>> >>>> A type of Visiter class that is used to walk a qobject's >>>> structure and assign each entry to the corresponding native C type. >>>> Command marshaling function will use this to pull out QMP command >>>> parameters recieved over the wire and pass them as native arguments >>>> to the corresponding C functions. >>>> >>>> Signed-off-by: Michael Roth >>>> --- >>>> qapi/qmp-input-visiter.c | 239 ++++++++++++++++++++++++++++++++++++++++++++++ >>>> qapi/qmp-input-visiter.h | 26 +++++ >>>> 2 files changed, 265 insertions(+), 0 deletions(-) >>>> create mode 100644 qapi/qmp-input-visiter.c >>>> create mode 100644 qapi/qmp-input-visiter.h >>>> >>>> diff --git a/qapi/qmp-input-visiter.c b/qapi/qmp-input-visiter.c >>>> new file mode 100644 >>>> index 0000000..6767e39 >>>> --- /dev/null >>>> +++ b/qapi/qmp-input-visiter.c >>>> @@ -0,0 +1,239 @@ >>>> +#include "qmp-input-visiter.h" >>>> +#include "qemu-queue.h" >>>> +#include "qemu-common.h" >>>> +#include "qemu-objects.h" >>>> +#include "qerror.h" >>>> + >>>> +#define QAPI_OBJECT_SIZE 512 >>>> + >>>> +#define QIV_STACK_SIZE 1024 >>>> + >>>> +typedef struct StackObject >>>> +{ >>>> + QObject *obj; >>>> + QListEntry *entry; >>>> +} StackObject; >>>> + >>>> +struct QmpInputVisiter >>>> +{ >>>> + Visiter visiter; >>>> + QObject *obj; >>>> + StackObject stack[QIV_STACK_SIZE]; >>>> + int nb_stack; >>>> +}; >>>> + >>>> +static QmpInputVisiter *to_qiv(Visiter *v) >>>> +{ >>>> + return container_of(v, QmpInputVisiter, visiter); >>>> +} >>>> + >>>> +static QObject *qmp_input_get_object(QmpInputVisiter *qiv, const char *name) >>>> +{ >>>> + QObject *qobj; >>>> + >>>> + if (qiv->nb_stack == 0) { >>>> + qobj = qiv->obj; >>>> + } else { >>>> + qobj = qiv->stack[qiv->nb_stack - 1].obj; >>>> + } >>>> + >>>> + if (name&& qobject_type(qobj) == QTYPE_QDICT) { >>>> + return qdict_get(qobject_to_qdict(qobj), name); >>>> + } else if (qiv->nb_stack> 0&& qobject_type(qobj) == QTYPE_QLIST) { >>>> + return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry); >>>> + } >>>> + >>>> + return qobj; >>>> +} >>>> + >>>> +static void qmp_input_push(QmpInputVisiter *qiv, QObject *obj) >>>> +{ >>>> + qiv->stack[qiv->nb_stack].obj = obj; >>>> + if (qobject_type(obj) == QTYPE_QLIST) { >>>> + qiv->stack[qiv->nb_stack].entry = qlist_first(qobject_to_qlist(obj)); >>>> + } >>>> + qiv->nb_stack++; >>>> + >>>> + assert(qiv->nb_stack< QIV_STACK_SIZE); // FIXME >>> >>> Can't this limit be reached if a client sends a nested object? Why >>> don't we make it dynamic and/or return an error if a limit is reached? >>> >> >> Yup, I think that's what the fixme was for. It's been fixed in my tree, >> just sets an Error and returns instead now. >> >> In reality the token limit added to the json parser with the set1 >> patches would catch overrun attempts from the client though, so it's >> just an extra layer of protection. > > Isn't this limit only in effect for individual tokens? Or does it also > catches the number of values in a dict? > You're right, looking again we have a limit on individual token size, and a limit on nested levels to avoid arbitrary levels of recursion in the parser. But the the number of entries that can be put into a dict/list are unbounded, so this check might actually needed. We'll probably want to harden the parser/streamer stuff for case as well, to the point where we can put an upper bound on how large a QMP client-produced qobject can be. I'll look at a separate patch against master for this. >> >>>> +} >>>> + >>>> +static void qmp_input_pop(QmpInputVisiter *qiv) >>>> +{ >>>> + qiv->nb_stack--; >>>> + assert(qiv->nb_stack>= 0); // FIXME >>>> +} >>>> + >>>> +static void qmp_input_start_struct(Visiter *v, void **obj, const char *kind, const char *name, Error **errp) >>>> +{ >>>> + QmpInputVisiter *qiv = to_qiv(v); >>>> + QObject *qobj = qmp_input_get_object(qiv, name); >>>> + >>>> + if (!qobj || qobject_type(qobj) != QTYPE_QDICT) { >>>> + error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, "object"); >>>> + return; >>>> + } >>>> + >>>> + qmp_input_push(qiv, qobj); >>>> + >>>> + if (obj) { >>>> + *obj = qemu_mallocz(QAPI_OBJECT_SIZE); >>> >>> I'm not sure I understand how this is being handled. This is allocating >>> the struct size, right? What happens if struct size> QAPI_OBJECT_SIZE? >>> >> >> Badness :) We'll need to pick a reasonable value and note it in the >> schema documentation. > > Isn't it possible to pass the struct size to visit_start_struct()? The Indeed! The generated code knows the struct type, so it could pass in the size as a parameter. I'll take a look at this. > object itself is passed... Another (complex) solution would be to walk > through the dictionary's elements and calculate the struct size. > The walking approach might have issues with optional fields and whatnot. >> >>>> + } >>>> +} >>>> + >>>> +static void qmp_input_end_struct(Visiter *v, Error **errp) >>>> +{ >>>> + QmpInputVisiter *qiv = to_qiv(v); >>>> + >>>> + qmp_input_pop(qiv); >>>> +} >>>> + >>>> +static void qmp_input_start_list(Visiter *v, const char *name, Error **errp) >>>> +{ >>>> + QmpInputVisiter *qiv = to_qiv(v); >>>> + QObject *qobj = qmp_input_get_object(qiv, name); >>>> + >>>> + if (!qobj || qobject_type(qobj) != QTYPE_QLIST) { >>>> + error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, "list"); >>>> + return; >>>> + } >>>> + >>>> + qmp_input_push(qiv, qobj); >>>> +} >>>> + >>>> +static GenericList *qmp_input_next_list(Visiter *v, GenericList **list, Error **errp) >>>> +{ >>>> + QmpInputVisiter *qiv = to_qiv(v); >>>> + GenericList *entry; >>>> + StackObject *so =&qiv->stack[qiv->nb_stack - 1]; >>>> + >>>> + if (so->entry == NULL) { >>>> + return NULL; >>>> + } >>>> + >>>> + entry = qemu_mallocz(sizeof(*entry)); >>>> + if (*list) { >>>> + so->entry = qlist_next(so->entry); >>>> + if (so->entry == NULL) { >>>> + qemu_free(entry); >>>> + return NULL; >>>> + } >>>> + (*list)->next = entry; >>>> + } >>>> + *list = entry; >>>> + >>>> + >>>> + return entry; >>>> +} >>>> + >>>> +static void qmp_input_end_list(Visiter *v, Error **errp) >>>> +{ >>>> + QmpInputVisiter *qiv = to_qiv(v); >>>> + >>>> + qmp_input_pop(qiv); >>>> +} >>>> + >>>> +static void qmp_input_type_int(Visiter *v, int64_t *obj, const char *name, Error **errp) >>>> +{ >>>> + QmpInputVisiter *qiv = to_qiv(v); >>>> + QObject *qobj = qmp_input_get_object(qiv, name); >>>> + >>>> + if (!qobj || qobject_type(qobj) != QTYPE_QINT) { >>>> + error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, "integer"); >>>> + return; >>>> + } >>>> + >>>> + *obj = qint_get_int(qobject_to_qint(qobj)); >>>> +} >>>> + >>>> +static void qmp_input_type_bool(Visiter *v, bool *obj, const char *name, Error **errp) >>>> +{ >>>> + QmpInputVisiter *qiv = to_qiv(v); >>>> + QObject *qobj = qmp_input_get_object(qiv, name); >>>> + >>>> + if (!qobj || qobject_type(qobj) != QTYPE_QBOOL) { >>>> + error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, "boolean"); >>>> + return; >>>> + } >>>> + >>>> + *obj = qbool_get_int(qobject_to_qbool(qobj)); >>>> +} >>>> + >>>> +static void qmp_input_type_str(Visiter *v, char **obj, const char *name, Error **errp) >>>> +{ >>>> + QmpInputVisiter *qiv = to_qiv(v); >>>> + QObject *qobj = qmp_input_get_object(qiv, name); >>>> + >>>> + if (!qobj || qobject_type(qobj) != QTYPE_QSTRING) { >>>> + error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, "string"); >>>> + return; >>>> + } >>>> + >>>> + *obj = qemu_strdup(qstring_get_str(qobject_to_qstring(qobj))); >>>> +} >>>> + >>>> +static void qmp_input_type_number(Visiter *v, double *obj, const char *name, Error **errp) >>>> +{ >>>> + QmpInputVisiter *qiv = to_qiv(v); >>>> + QObject *qobj = qmp_input_get_object(qiv, name); >>>> + >>>> + if (!qobj || qobject_type(qobj) != QTYPE_QFLOAT) { >>>> + error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, "double"); >>>> + return; >>>> + } >>>> + >>>> + *obj = qfloat_get_double(qobject_to_qfloat(qobj)); >>>> +} >>>> + >>>> +static void qmp_input_type_enum(Visiter *v, int *obj, const char *kind, const char *name, Error **errp) >>>> +{ >>>> + int64_t value; >>>> + qmp_input_type_int(v,&value, name, errp); >>>> + *obj = value; >>>> +} >>>> + >>>> +static void qmp_input_start_optional(Visiter *v, bool *present, >>>> + const char *name, Error **errp) >>>> +{ >>>> + QmpInputVisiter *qiv = to_qiv(v); >>>> + QObject *qobj = qmp_input_get_object(qiv, name); >>>> + >>>> + if (!qobj) { >>>> + *present = false; >>>> + return; >>>> + } >>>> + >>>> + *present = true; >>>> +} >>>> + >>>> +static void qmp_input_end_optional(Visiter *v, Error **errp) >>>> +{ >>>> +} >>>> + >>>> +Visiter *qmp_input_get_visiter(QmpInputVisiter *v) >>>> +{ >>>> + return&v->visiter; >>>> +} >>>> + >>>> +QmpInputVisiter *qmp_input_visiter_new(QObject *obj) >>>> +{ >>>> + QmpInputVisiter *v; >>>> + >>>> + v = qemu_mallocz(sizeof(*v)); >>>> + >>>> + v->visiter.start_struct = qmp_input_start_struct; >>>> + v->visiter.end_struct = qmp_input_end_struct; >>>> + v->visiter.start_list = qmp_input_start_list; >>>> + v->visiter.next_list = qmp_input_next_list; >>>> + v->visiter.end_list = qmp_input_end_list; >>>> + v->visiter.type_enum = qmp_input_type_enum; >>>> + v->visiter.type_int = qmp_input_type_int; >>>> + v->visiter.type_bool = qmp_input_type_bool; >>>> + v->visiter.type_str = qmp_input_type_str; >>>> + v->visiter.type_number = qmp_input_type_number; >>>> + v->visiter.start_optional = qmp_input_start_optional; >>>> + v->visiter.end_optional = qmp_input_end_optional; >>>> + >>>> + v->obj = obj; >>>> + >>>> + return v; >>>> +} >>>> diff --git a/qapi/qmp-input-visiter.h b/qapi/qmp-input-visiter.h >>>> new file mode 100644 >>>> index 0000000..3e4d06f >>>> --- /dev/null >>>> +++ b/qapi/qmp-input-visiter.h >>>> @@ -0,0 +1,26 @@ >>>> +/* >>>> + * Input Visiter >>>> + * >>>> + * Copyright IBM, Corp. 2011 >>>> + * >>>> + * Authors: >>>> + * Anthony Liguori >>>> + * >>>> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. >>>> + * See the COPYING.LIB file in the top-level directory. >>>> + * >>>> + */ >>>> + >>>> +#ifndef QMP_INPUT_VISITER_H >>>> +#define QMP_INPUT_VISITER_H >>>> + >>>> +#include "qapi-visit-core.h" >>>> +#include "qobject.h" >>>> + >>>> +typedef struct QmpInputVisiter QmpInputVisiter; >>>> + >>>> +QmpInputVisiter *qmp_input_visiter_new(QObject *obj); >>>> + >>>> +Visiter *qmp_input_get_visiter(QmpInputVisiter *v); >>>> + >>>> +#endif >>> >> >