From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:35692) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QgdIH-0002SN-8I for qemu-devel@nongnu.org; Tue, 12 Jul 2011 09:46:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QgdID-0006nE-Il for qemu-devel@nongnu.org; Tue, 12 Jul 2011 09:46:32 -0400 Received: from e33.co.us.ibm.com ([32.97.110.151]:35356) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QgdIC-0006me-Iy for qemu-devel@nongnu.org; Tue, 12 Jul 2011 09:46:29 -0400 Received: from d03relay04.boulder.ibm.com (d03relay04.boulder.ibm.com [9.17.195.106]) by e33.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id p6CDcjTn011545 for ; Tue, 12 Jul 2011 07:38:45 -0600 Received: from d03av06.boulder.ibm.com (d03av06.boulder.ibm.com [9.17.195.245]) by d03relay04.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p6CDkEUW130240 for ; Tue, 12 Jul 2011 07:46:14 -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 p6CDpgJV029723 for ; Tue, 12 Jul 2011 07:51:42 -0600 Message-ID: <4E1C5025.4060007@linux.vnet.ibm.com> Date: Tue, 12 Jul 2011 08:46:13 -0500 From: Michael Roth MIME-Version: 1.0 References: <1309870965-27066-1-git-send-email-mdroth@linux.vnet.ibm.com> <1309870965-27066-6-git-send-email-mdroth@linux.vnet.ibm.com> <20110707113234.328bc500@doriath> <4E1B8FE6.8060202@linux.vnet.ibm.com> <20110712101631.22d636a8@doriath> In-Reply-To: <20110712101631.22d636a8@doriath> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 05/18] qapi: add QMP input visitor 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 07/12/2011 08:16 AM, Luiz Capitulino wrote: > On Mon, 11 Jul 2011 19:05:58 -0500 > Michael Roth wrote: > >> On 07/07/2011 09:32 AM, Luiz Capitulino wrote: >>> On Tue, 5 Jul 2011 08:02:32 -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 >>>> --- >>>> Makefile.objs | 2 +- >>>> qapi/qmp-input-visitor.c | 264 ++++++++++++++++++++++++++++++++++++++++++++++ >>>> qapi/qmp-input-visitor.h | 27 +++++ >>>> qerror.h | 3 + >>>> 4 files changed, 295 insertions(+), 1 deletions(-) >>>> create mode 100644 qapi/qmp-input-visitor.c >>>> create mode 100644 qapi/qmp-input-visitor.h >>>> >>>> diff --git a/Makefile.objs b/Makefile.objs >>>> index 0077014..997ecef 100644 >>>> --- a/Makefile.objs >>>> +++ b/Makefile.objs >>>> @@ -375,7 +375,7 @@ libcacard-y = cac.o event.o vcard.o vreader.o vcard_emul_nss.o vcard_emul_type.o >>>> ###################################################################### >>>> # qapi >>>> >>>> -qapi-nested-y = qapi-visit-core.o >>>> +qapi-nested-y = qapi-visit-core.o qmp-input-visitor.o >>>> qapi-obj-y = $(addprefix qapi/, $(qapi-nested-y)) >>>> >>>> vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS) >>>> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c >>>> new file mode 100644 >>>> index 0000000..80912bb >>>> --- /dev/null >>>> +++ b/qapi/qmp-input-visitor.c >>>> @@ -0,0 +1,264 @@ >>>> +/* >>>> + * Input Visitor >>>> + * >>>> + * 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. >>>> + * >>>> + */ >>>> + >>>> +#include "qmp-input-visitor.h" >>>> +#include "qemu-queue.h" >>>> +#include "qemu-common.h" >>>> +#include "qemu-objects.h" >>>> +#include "qerror.h" >>>> + >>>> +#define QIV_STACK_SIZE 1024 >>>> + >>>> +typedef struct StackObject >>>> +{ >>>> + const QObject *obj; >>>> + const QListEntry *entry; >>>> +} StackObject; >>>> + >>>> +struct QmpInputVisitor >>>> +{ >>>> + Visitor visitor; >>>> + const QObject *obj; >>>> + StackObject stack[QIV_STACK_SIZE]; >>>> + int nb_stack; >>>> +}; >>>> + >>>> +static QmpInputVisitor *to_qiv(Visitor *v) >>>> +{ >>>> + return container_of(v, QmpInputVisitor, visitor); >>>> +} >>>> + >>>> +static const QObject *qmp_input_get_object(QmpInputVisitor *qiv, const char *name) >>>> +{ >>>> + const 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(QmpInputVisitor *qiv, const QObject *obj, Error **errp) >>>> +{ >>>> + 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++; >>>> + >>>> + if (qiv->nb_stack>= QIV_STACK_SIZE) { >>>> + error_set(errp, QERR_BUFFER_OVERRUN); >>>> + return; >>>> + } >>>> +} >>>> + >>>> +static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp) >>>> +{ >>>> + qiv->nb_stack--; >>>> + if (qiv->nb_stack< 0) { >>>> + error_set(errp, QERR_BUFFER_OVERRUN); >>>> + return; >>>> + } >>>> +} >>>> + >>>> +static void qmp_input_start_struct(Visitor *v, void **obj, const char *kind, const char *name, size_t size, Error **errp) >>>> +{ >>>> + QmpInputVisitor *qiv = to_qiv(v); >>>> + const QObject *qobj = qmp_input_get_object(qiv, name); >>>> + >>>> + if (!qobj || qobject_type(qobj) != QTYPE_QDICT) { >>>> + error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "QDict"); >>>> + return; >>>> + } >>>> + >>>> + qmp_input_push(qiv, qobj, errp); >>>> + if (error_is_set(errp)) { >>>> + return; >>>> + } >>>> + >>>> + if (obj) { >>>> + *obj = qemu_mallocz(size); >>>> + } >>>> +} >>>> + >>>> +static void qmp_input_end_struct(Visitor *v, Error **errp) >>>> +{ >>>> + QmpInputVisitor *qiv = to_qiv(v); >>>> + >>>> + qmp_input_pop(qiv, errp); >>>> +} >>>> + >>>> +static void qmp_input_start_list(Visitor *v, const char *name, Error **errp) >>>> +{ >>>> + QmpInputVisitor *qiv = to_qiv(v); >>>> + const QObject *qobj = qmp_input_get_object(qiv, name); >>>> + >>>> + if (!qobj || qobject_type(qobj) != QTYPE_QLIST) { >>>> + error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "list"); >>>> + return; >>>> + } >>>> + >>>> + qmp_input_push(qiv, qobj, errp); >>>> +} >>>> + >>>> +static GenericList *qmp_input_next_list(Visitor *v, GenericList **list, Error **errp) >>>> +{ >>>> + QmpInputVisitor *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(Visitor *v, Error **errp) >>>> +{ >>>> + QmpInputVisitor *qiv = to_qiv(v); >>>> + >>>> + qmp_input_pop(qiv, errp); >>>> +} >>>> + >>>> +static void qmp_input_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp) >>>> +{ >>>> + QmpInputVisitor *qiv = to_qiv(v); >>>> + const QObject *qobj = qmp_input_get_object(qiv, name); >>>> + >>>> + if (!qobj || qobject_type(qobj) != QTYPE_QINT) { >>>> + error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "integer"); >>>> + return; >>>> + } >>>> + >>>> + *obj = qint_get_int(qobject_to_qint(qobj)); >>>> +} >>>> + >>>> +static void qmp_input_type_bool(Visitor *v, bool *obj, const char *name, Error **errp) >>>> +{ >>>> + QmpInputVisitor *qiv = to_qiv(v); >>>> + const QObject *qobj = qmp_input_get_object(qiv, name); >>>> + >>>> + if (!qobj || qobject_type(qobj) != QTYPE_QBOOL) { >>>> + error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "boolean"); >>>> + return; >>>> + } >>>> + >>>> + *obj = qbool_get_int(qobject_to_qbool(qobj)); >>>> +} >>>> + >>>> +static void qmp_input_type_str(Visitor *v, char **obj, const char *name, Error **errp) >>>> +{ >>>> + QmpInputVisitor *qiv = to_qiv(v); >>>> + const QObject *qobj = qmp_input_get_object(qiv, name); >>>> + >>>> + if (!qobj || qobject_type(qobj) != QTYPE_QSTRING) { >>>> + error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "string"); >>>> + return; >>>> + } >>>> + >>>> + *obj = qemu_strdup(qstring_get_str(qobject_to_qstring(qobj))); >>>> +} >>>> + >>>> +static void qmp_input_type_number(Visitor *v, double *obj, const char *name, Error **errp) >>>> +{ >>>> + QmpInputVisitor *qiv = to_qiv(v); >>>> + const QObject *qobj = qmp_input_get_object(qiv, name); >>>> + >>>> + if (!qobj || qobject_type(qobj) != QTYPE_QFLOAT) { >>>> + error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "double"); >>>> + return; >>>> + } >>>> + >>>> + *obj = qfloat_get_double(qobject_to_qfloat(qobj)); >>>> +} >>>> + >>>> +static void qmp_input_type_enum(Visitor *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(Visitor *v, bool *present, >>>> + const char *name, Error **errp) >>>> +{ >>>> + QmpInputVisitor *qiv = to_qiv(v); >>>> + const QObject *qobj = qmp_input_get_object(qiv, name); >>>> + >>>> + if (!qobj) { >>>> + *present = false; >>>> + return; >>>> + } >>>> + >>>> + *present = true; >>>> +} >>>> + >>>> +static void qmp_input_end_optional(Visitor *v, Error **errp) >>>> +{ >>>> +} >>>> + >>>> +Visitor *qmp_input_get_visitor(QmpInputVisitor *v) >>>> +{ >>>> + return&v->visitor; >>>> +} >>>> + >>>> +void qmp_input_visitor_cleanup(QmpInputVisitor *v) >>>> +{ >>> >>> We talked about doing a qobject_incref(v->obj) in qmp_input_visitor_new() and >>> decrefing here. >>> >> >> I think it was either that, or just making the QObject arg to >> qmp_input_visitor_new() const, to make it clear it was caller-freed. > > No, because (at least theoretically) a client could free it before walking > the struct with the visitor functions. The rule is: if you store it, you > have to increment its reference or get its ownership. > > Makes sense. Sucks to have to remove the const for that though...would a temporary cast to non-const be acceptable in a case like this? >> >>>> + qemu_free(v); >>>> +} >>>> + >>>> +QmpInputVisitor *qmp_input_visitor_new(const QObject *obj) >>>> +{ >>>> + QmpInputVisitor *v; >>>> + >>>> + v = qemu_mallocz(sizeof(*v)); >>>> + >>>> + v->visitor.start_struct = qmp_input_start_struct; >>>> + v->visitor.end_struct = qmp_input_end_struct; >>>> + v->visitor.start_list = qmp_input_start_list; >>>> + v->visitor.next_list = qmp_input_next_list; >>>> + v->visitor.end_list = qmp_input_end_list; >>>> + v->visitor.type_enum = qmp_input_type_enum; >>>> + v->visitor.type_int = qmp_input_type_int; >>>> + v->visitor.type_bool = qmp_input_type_bool; >>>> + v->visitor.type_str = qmp_input_type_str; >>>> + v->visitor.type_number = qmp_input_type_number; >>>> + v->visitor.start_optional = qmp_input_start_optional; >>>> + v->visitor.end_optional = qmp_input_end_optional; >>>> + >>>> + v->obj = obj; >>>> + >>>> + return v; >>>> +} >>>> diff --git a/qapi/qmp-input-visitor.h b/qapi/qmp-input-visitor.h >>>> new file mode 100644 >>>> index 0000000..0f77916 >>>> --- /dev/null >>>> +++ b/qapi/qmp-input-visitor.h >>>> @@ -0,0 +1,27 @@ >>>> +/* >>>> + * Input Visitor >>>> + * >>>> + * 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_VISITOR_H >>>> +#define QMP_INPUT_VISITOR_H >>>> + >>>> +#include "qapi-visit-core.h" >>>> +#include "qobject.h" >>>> + >>>> +typedef struct QmpInputVisitor QmpInputVisitor; >>>> + >>>> +QmpInputVisitor *qmp_input_visitor_new(const QObject *obj); >>>> +void qmp_input_visitor_cleanup(QmpInputVisitor *v); >>>> + >>>> +Visitor *qmp_input_get_visitor(QmpInputVisitor *v); >>>> + >>>> +#endif >>>> diff --git a/qerror.h b/qerror.h >>>> index 16c830d..9a9fa5b 100644 >>>> --- a/qerror.h >>>> +++ b/qerror.h >>>> @@ -124,6 +124,9 @@ QError *qobject_to_qerror(const QObject *obj); >>>> #define QERR_JSON_PARSE_ERROR \ >>>> "{ 'class': 'JSONParseError', 'data': { 'message': %s } }" >>>> >>>> +#define QERR_BUFFER_OVERRUN \ >>>> + "{ 'class': 'BufferOverrun', 'data': {} }" >>>> + >>> >>> You also have to add an entry in qerror.c. I slightly prefer NotEnoughMemory, >>> but BufferOverrrun is probably fine. >>> >>>> #define QERR_KVM_MISSING_CAP \ >>>> "{ 'class': 'KVMMissingCap', 'data': { 'capability': %s, 'feature': %s } }" >>>> >>> >> >