Re: [Qemu-devel] [PATCH v3 05/21] qapi: add QMP input visitor

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 15 Jun 2011 11:56:03 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On 06/15/2011 11:11 AM, Luiz Capitulino wrote:
> > On Mon, 13 Jun 2011 21:31:10 -0500
> > Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
> >
> <snip>
> >> +Visitor *qmp_input_get_visitor(QmpInputVisitor *v)
> >> +{
> >> +    return&v->visitor;
> >> +}
> >> +
> >> +void qmp_input_visitor_cleanup(QmpInputVisitor *v)
> >> +{
> >
> > You're not decrementing the reference to v->obj. Actually, we should also
> > increment the reference to it before storing or document that the reference
> > ownership is transferred to this function.
> >
> 
> Ownership never gets transferred to the visitor in the dispatch path; 
> the caller is supposed to free it. I thought there was some manipulation 
> elsewhere that prevented us from using a const argument to 
> qmp_input_visitor_new() instead, but it looks like we can. Otherwise, 
> I'd prefer to do an inc+dec to make this more explicit.

Yeah, I prefer that too.

> 
> >> +    qemu_free(v);
> >> +}
> >> +
> >> +QmpInputVisitor *qmp_input_visitor_new(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..3f798f0
> >> --- /dev/null
> >> +++ b/qapi/qmp-input-visitor.h
> >> @@ -0,0 +1,27 @@
> >> +/*
> >> + * Input Visitor
> >> + *
> >> + * 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_VISITOR_H
> >> +#define QMP_INPUT_VISITOR_H
> >> +
> >> +#include "qapi-visit-core.h"
> >> +#include "qobject.h"
> >> +
> >> +typedef struct QmpInputVisitor QmpInputVisitor;
> >> +
> >> +QmpInputVisitor *qmp_input_visitor_new(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..7a89a50 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_QAPI_VISITOR_STACK_OVERRUN \
> >> +    "{ 'class': 'QAPIVisitorStackOverrun', 'data': {} }"
> >> +
> >>   #define QERR_KVM_MISSING_CAP \
> >>       "{ 'class': 'KVMMissingCap', 'data': { 'capability': %s, 'feature': %s } }"
> >>
> >
>