Re: [Qemu-devel] [PATCH v4 05/28] qapi: Add new visit_free() function

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Making each visitor provide its own (awkwardly-named) FOO_cleanup()
> is unusual, when we can instead have a polymorphic visit_free()
> interface.
>
> The dealloc visitor is the first one converted to completely use
> the new entry point, since only generated code and the testsuite
> were using it.  Diffs to the generated code look like:
>
> | void qapi_free_ACPIOSTInfo(ACPIOSTInfo *obj)
> | {
> |-    QapiDeallocVisitor *qdv;
> |     Visitor *v;
> |
> |     if (!obj) {
> |         return;
> |     }
> |
> |-    qdv = qapi_dealloc_visitor_new();
> |-    v = qapi_dealloc_get_visitor(qdv);
> |+    v = qapi_dealloc_visitor_new();

At this point, I wonder what this change has to do with the new
visit_free().  It turns out that...

> |     visit_type_ACPIOSTInfo(v, NULL, &obj, NULL);
> |-    qapi_dealloc_visitor_cleanup(qdv);

... this call is the only use of qdv, so you're eliminating it.

Next, I wonder whether this is worthwhile, as it creates an
inconsistency with the other visitors.  Peeking ahead, I see the
inconsistency is temporary: you're eliminating the other _get_visitor()
later in this series.  Okay, but perhaps you can explain your intentions
further up in the commit message.

> |+    visit_free(v);
> |}
>
> Signed-off-by: Eric Blake <address@hidden>
[...]
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index dcfbf92..28d2203 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -513,6 +513,14 @@ opts_optional(Visitor *v, const char *name, bool 
> *present)
>  }
>
>
> +static void
> +opts_free(Visitor *v)
> +{
> +    OptsVisitor *ov = to_ov(v);
> +    opts_visitor_cleanup(ov);

I'd eliminate the variable:

       opts_visitor_cleanup(to_ov(v));

Hmm, you put it to use when you inline opts_visitor_cleanup() in the
next patch.  Doesn't matter then.

> +}
> +
> +
>  OptsVisitor *
>  opts_visitor_new(const QemuOpts *opts)
>  {
> @@ -540,6 +548,7 @@ opts_visitor_new(const QemuOpts *opts)
>       * skip some mandatory methods... */
>
>      ov->visitor.optional = &opts_optional;
> +    ov->visitor.free = opts_free;
>
>      ov->opts_root = opts;
>
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index 9391dea..235e8a1 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -107,17 +107,12 @@ static void qapi_dealloc_type_null(Visitor *v, const 
> char *name, Error **errp)
>  {
>  }
>
> -Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v)
> -{
> -    return &v->visitor;
> -}
> -
> -void qapi_dealloc_visitor_cleanup(QapiDeallocVisitor *v)
> +static void qapi_dealloc_free(Visitor *v)
>  {
>      g_free(v);

Uh, shouldn't this be g_free(v, QapiDeallocVisitor, visitor)?  That way,
we don't assume that visitor is QapiDeallocVisitor's first member.

>  }
>
> -QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
> +Visitor *qapi_dealloc_visitor_new(void)
>  {
>      QapiDeallocVisitor *v;
>
> @@ -138,6 +133,7 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
>      v->visitor.type_number = qapi_dealloc_type_number;
>      v->visitor.type_any = qapi_dealloc_type_anything;
>      v->visitor.type_null = qapi_dealloc_type_null;
> +    v->visitor.free = qapi_dealloc_free;
>
> -    return v;
> +    return &v->visitor;
>  }
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 84f32fc..3ca192e 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -375,6 +375,12 @@ Visitor *qmp_input_get_visitor(QmpInputVisitor *v)
>      return &v->visitor;
>  }
>
> +static void qmp_input_free(Visitor *v)
> +{
> +    QmpInputVisitor *qiv = to_qiv(v);
> +    qmp_input_visitor_cleanup(qiv);
> +}
> +

Like for opts_free(), the variable becomes useful when
qmp_input_visitor_cleanup() gets inlined in a later patch.

Same for the other free methods.

>  void qmp_input_visitor_cleanup(QmpInputVisitor *v)
>  {
>      qobject_decref(v->root);
> @@ -403,6 +409,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj, bool 
> strict)
>      v->visitor.type_any = qmp_input_type_any;
>      v->visitor.type_null = qmp_input_type_null;
>      v->visitor.optional = qmp_input_optional;
> +    v->visitor.free = qmp_input_free;
>      v->strict = strict;
>
>      v->root = obj;
[...]