Re: [Qemu-devel] [PATCH v2][ 10/21] qapi: add QMP input visiter

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On 06/09/2011 10:55 AM, Luiz Capitulino wrote:
> On Thu, 09 Jun 2011 10:41:53 -0500
> Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
>
>> On 06/09/2011 10:30 AM, Luiz Capitulino wrote:
>>> On Fri,  3 Jun 2011 17:33:08 -0500
>>> Michael Roth<mdroth@linux.vnet.ibm.com>   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<mdroth@linux.vnet.ibm.com>
>>>> ---
>>>>    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<aliguori@us.ibm.com>
>>>> + *
>>>> + * 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
>>>
>>
>