From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:34340) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QUl7p-0002rZ-SM for qemu-devel@nongnu.org; Thu, 09 Jun 2011 15:42:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QUl7n-0007pa-MF for qemu-devel@nongnu.org; Thu, 09 Jun 2011 15:42:41 -0400 Received: from mail-yx0-f173.google.com ([209.85.213.173]:39394) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QUl7n-0007pG-7i for qemu-devel@nongnu.org; Thu, 09 Jun 2011 15:42:39 -0400 Received: by yxn22 with SMTP id 22so91838yxn.4 for ; Thu, 09 Jun 2011 12:42:38 -0700 (PDT) Message-ID: <4DF1222B.9050506@codemonkey.ws> Date: Thu, 09 Jun 2011 14:42:35 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1307140399-9023-1-git-send-email-mdroth@linux.vnet.ibm.com> <1307140399-9023-12-git-send-email-mdroth@linux.vnet.ibm.com> <20110609144743.7d74aa46@doriath> In-Reply-To: <20110609144743.7d74aa46@doriath> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2][ 11/21] qapi: add QMP output visiter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: Jes.Sorensen@redhat.com, agl@linux.vnet.ibm.com, Michael Roth , qemu-devel@nongnu.org On 06/09/2011 12:47 PM, Luiz Capitulino wrote: > On Fri, 3 Jun 2011 17:33:09 -0500 > Michael Roth 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 >> --- >> 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