Re: [Qemu-devel] [PATCH v2][ 11/21] qapi: add QMP output visiter

[Anthony Liguori]

On 06/09/2011 12:47 PM, Luiz Capitulino wrote:
> On Fri,  3 Jun 2011 17:33:09 -0500
> Michael Roth<address@hidden>  wrote:
>
> > Type of Visiter class that serves as the inverse of the input visiter:
> > it takes a series of native C types and uses their values to construct a
> > corresponding QObject. The command marshaling/dispatcher functions will
> > use this to convert the output of QMP functions into a QObject that can
> > be sent over the wire.
> >
> > Signed-off-by: Michael Roth<address@hidden>
> > ---
> >   qapi/qmp-output-visiter.c |  180 +++++++++++++++++++++++++++++++++++++++++++++
> >   qapi/qmp-output-visiter.h |   27 +++++++
> >   2 files changed, 207 insertions(+), 0 deletions(-)
> >   create mode 100644 qapi/qmp-output-visiter.c
> >   create mode 100644 qapi/qmp-output-visiter.h
> >
> > diff --git a/qapi/qmp-output-visiter.c b/qapi/qmp-output-visiter.c
> > new file mode 100644
> > index 0000000..4a7cb36
> > --- /dev/null
> > +++ b/qapi/qmp-output-visiter.c
> > @@ -0,0 +1,180 @@
> > +#include "qmp-output-visiter.h"
> > +#include "qemu-queue.h"
> > +#include "qemu-common.h"
> > +#include "qemu-objects.h"
> > +
> > +typedef struct QStackEntry
> > +{
> > +    QObject *value;
> > +    QTAILQ_ENTRY(QStackEntry) node;
> > +} QStackEntry;
> > +
> > +typedef QTAILQ_HEAD(QStack, QStackEntry) QStack;
> > +
> > +struct QmpOutputVisiter
> > +{
> > +    Visiter visiter;
> > +    QStack stack;
> > +};
> > +
> > +#define qmp_output_add(qov, name, value) qmp_output_add_obj(qov, name, 
> > QOBJECT(value))
> > +#define qmp_output_push(qov, value) qmp_output_push_obj(qov, QOBJECT(value))
> > +
> > +static QmpOutputVisiter *to_qov(Visiter *v)
> > +{
> > +    return container_of(v, QmpOutputVisiter, visiter);
> > +}
> > +
> > +static void qmp_output_push_obj(QmpOutputVisiter *qov, QObject *value)
> > +{
> > +    QStackEntry *e = qemu_mallocz(sizeof(*e));
> > +
> > +    e->value = value;
> > +    QTAILQ_INSERT_HEAD(&qov->stack, e, node);
> > +}
> > +
> > +static QObject *qmp_output_pop(QmpOutputVisiter *qov)
> > +{
> > +    QStackEntry *e = QTAILQ_FIRST(&qov->stack);
> > +    QObject *value;
> > +    QTAILQ_REMOVE(&qov->stack, e, node);
> > +    value = e->value;
> > +    qemu_free(e);
> > +    return value;
> > +}
> > +
> > +static QObject *qmp_output_first(QmpOutputVisiter *qov)
> > +{
> > +    QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack);
> > +    return e->value;
> > +}
> > +
> > +static QObject *qmp_output_last(QmpOutputVisiter *qov)
> > +{
> > +    QStackEntry *e = QTAILQ_FIRST(&qov->stack);
> > +    return e->value;
> > +}
> > +
> > +static void qmp_output_add_obj(QmpOutputVisiter *qov, const char *name, 
> > QObject *value)
> > +{
> > +    QObject *cur;
> > +
> > +    if (QTAILQ_EMPTY(&qov->stack)) {
> > +        qmp_output_push_obj(qov, value);
> > +        return;
> > +    }
> > +
> > +    cur = qmp_output_last(qov);
> > +
> > +    switch (qobject_type(cur)) {
> > +    case QTYPE_QDICT:
> > +        qdict_put_obj(qobject_to_qdict(cur), name, value);
> > +        break;
> > +    case QTYPE_QLIST:
> > +        qlist_append_obj(qobject_to_qlist(cur), value);
> > +        break;
> > +    default:
> > +        qobject_decref(qmp_output_pop(qov));
>
> I'm not sure I understand when we should discard an item like this,
> shouldn't this be an assert()?
>
> > +        qmp_output_push_obj(qov, value);
> > +        break;
> > +    }
> > +}
> > +
> > +static void qmp_output_start_struct(Visiter *v, void **obj, const char *kind, 
> > const char *name, Error **errp)
> > +{
> > +    QmpOutputVisiter *qov = to_qov(v);
> > +    QDict *dict = qdict_new();
> > +
> > +    qmp_output_add(qov, name, dict);
>
> This calls qmp_output_push() if the stack is empty...
>
> > +    qmp_output_push(qov, dict);
>
> ...then it's called again here, so the element is added twice,
> is this expected?

There's a subtle behavior here that both of your comments are asking about.

Let's say you're trying to generate:

{ 'hello': { 'world': true } }

It'll generate a sequence of calls like:

1. start_struct('')
2. start_struct('hello')
3. marshal_bool('world')
4. end_struct()
5. end_struct()

At call (1), you've got an empty stack.  You need to start with 
something, so you start by pushing the element onto the stack.

This is sort of the boot strap bit to get a first element on the stack. 
 This is really designed to work with the following sequence in mind:

1. marshal_bool('world')
2. marshal_bool('foo')

In this case, the semantics are that the top-of-stack after (1) is bool 
world, but then after (2), that object is freed, and then the 
top-of-stack is bool foo.

This results in the slightly odd behavior of adding a dictionary twice 
to the stack when it's the top-level element but the behavior is 
entirely intentional.

You need to add it twice such that when you pop it on end_struct, you 
still have a reference that you can return in qmp_output_visitor_get_obj().

Regards,

Anthony Liguori