qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v9 05/37] vl: Improve use of qapi visitor
Date: Wed, 20 Jan 2016 14:43:20 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Eric Blake <address@hidden> 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).  Guarantee that visit_end_struct()
> is called if visit_start_struct() succeeded.

Three separate things --- you're pushing it now :)

Impact of not calling visit_end_struct()?

> Signed-off-by: Eric Blake <address@hidden>
> Reviewed-by: Marc-André Lureau <address@hidden>
>
> ---
> 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
> ---
>  vl.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index f043009..aaa5403 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2822,44 +2822,47 @@ static bool object_create_delayed(const char *type)
>  static int object_create(void *opaque, QemuOpts *opts, Error **errp)
>  {
>      Error *err = NULL;
> +    Error *err_end = NULL;
>      char *type = NULL;
>      char *id = NULL;
> -    void *dummy = NULL;
>      OptsVisitor *ov;
>      QDict *pdict;
>      bool (*type_predicate)(const char *) = opaque;
> +    Visitor *v;
>
>      ov = opts_visitor_new(opts);
>      pdict = qemu_opts_to_qdict(opts, NULL);
> +    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);

Same argument as for the previous patch.

>      if (err) {
>          goto out;
>      }
>
>      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;
>      }

If we get here, we must call visit_end_struct().

>      if (!type_predicate(type)) {
> +        visit_end_struct(v, NULL);

Here, you add the previously missing visit_end_struct() to the error
path.

>          goto out;
>      }
>
>      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;
> +        goto out_end;

Here, you skip to the function's cleanup, which now includes
visit_end_struct().

Such a mix is can be a sign the cleanup isn't quite in the right order.

When we take this error path to out_end, then:

   out_end:
       visit_end_struct(v, &err_end);   // called, as we must
       if (!err && err_end) {           // !err is false
           qmp_object_del(id, NULL);    // not called
       }
       error_propagate(&err, err_end);  // since err, this is
                                        // error_free(err_end)

>      }
>
> -    object_add(type, id, pdict, opts_get_visitor(ov), &err);
> -    if (err) {
> -        goto out;
> -    }
> -    visit_end_struct(opts_get_visitor(ov), &err);
> -    if (err) {
> +    object_add(type, id, pdict, v, &err);
> +
> +out_end:
> +    visit_end_struct(v, &err_end);

visit_end_struct() must be called when visit_start_struct() succeeded.
All error paths after that success pass through here, except for one,
and that one calls visit_end_struct().  Okay.

> +    if (!err && err_end) {
>          qmp_object_del(id, NULL);
>      }

qmp_object_del() must be called when we fail after object_add()
succeeded.  That's what the condition does.

> +    error_propagate(&err, err_end);
>
>  out:

Cleanup below here must be done always.

>      opts_visitor_cleanup(ov);

The only reason I'm not asking you to rewrite this mess is that you're
already rewriting it later in this series.

> @@ -2867,7 +2870,6 @@ out:
>      QDECREF(pdict);
>      g_free(id);
>      g_free(type);
> -    g_free(dummy);
>      if (err) {
>          error_report_err(err);
>          return -1;

I wonder whether splitting this and the previous patch along the change
rather than the file would come out nicer:

1. Cache the visitor

2. Suppress the pointless allocation

3. Fix to call visit_end_struct()



reply via email to

[Prev in Thread] Current Thread [Next in Thread]