Re: [Qemu-devel] [PATCH v9 04/37] hmp: Improve use of qapi visitor

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> Cache the visitor in a local variable instead of repeatedly
> calling the accessor.  Pass NULL for the visit_start_struct()
> object (which matches the fact that we were already passing 0
> for the size argument, because we aren't using the visit to
> allocate a qapi struct).

Two separate things.  Doing them both in a single patch is okay because
the resulting patch is short enough to be easily reviewable anyway.

> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> ---
> v9: no change
> v8: no change
> v7: place earlier in series, drop attempts to provide a 'kind' string,
> drop bogus avoidance of qmp_object_del() on error
> v6: new patch, split from RFC on v5 7/46
> ---
>  hmp.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index 54f2620..6d67f9b 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1656,9 +1656,9 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
>      QemuOpts *opts;
>      char *type = NULL;
>      char *id = NULL;
> -    void *dummy = NULL;
>      OptsVisitor *ov;
>      QDict *pdict;
> +    Visitor *v;
>
>      opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
>      if (err) {
> @@ -1667,28 +1667,29 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
>
>      ov = opts_visitor_new(opts);
>      pdict = qdict_clone_shallow(qdict);
> +    v = opts_get_visitor(ov);
>
> -    visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
> +    visit_start_struct(v, NULL, NULL, NULL, 0, &err);


The only callers that can allocate are the generated visit_type_FOO()
for struct or union FOO.

The non-allocating callers mostly look like

    visit_start_struct(v, NULL, NULL, NULL, 0, &err);

Some pass a non-null @kind (third argument), but that's rubbish anyway
(see PATCH 17).

Some pass a non-null @name (fourth argument).  QMP visitors need that,
others don't care.

This caller is the only one that passes a non-null @obj (second
argument).  Makes visitors capable of allocating allocate @size bytes
(fifth argument).  Except opts-visitor allocates one byte instead of
zero to avoid a null object, for unclear reasons.  Whatever gets
allocated we free (see last hunk) without using.  The patch avoids this
pointless allocation.  Good.  Suggest to explain this more clearly in
the commit message:

    Pass NULL for the visit_start_struct() parameter @obj to suppress
    unwanted allocation, like we do elsewhere.

>      if (err) {
>          goto out_clean;
>      }
>
>      qdict_del(pdict, "qom-type");
> -    visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err);
> +    visit_type_str(v, &type, "qom-type", &err);
>      if (err) {
>          goto out_end;
>      }
>
>      qdict_del(pdict, "id");
> -    visit_type_str(opts_get_visitor(ov), &id, "id", &err);
> +    visit_type_str(v, &id, "id", &err);
>      if (err) {
>          goto out_end;
>      }
>
> -    object_add(type, id, pdict, opts_get_visitor(ov), &err);
> +    object_add(type, id, pdict, v, &err);
>
>  out_end:
> -    visit_end_struct(opts_get_visitor(ov), &err_end);
> +    visit_end_struct(v, &err_end);
>      if (!err && err_end) {
>          qmp_object_del(id, NULL);
>      }
> @@ -1700,7 +1701,6 @@ out_clean:
>      qemu_opts_del(opts);
>      g_free(id);
>      g_free(type);
> -    g_free(dummy);
>
>  out:
>      hmp_handle_error(mon, &err);