Re: [Qemu-devel] [PATCH v9 08/37] qapi: Track all failures between visit_start/stop

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Inside the generated code between visit_start_struct() and
> visit_end_struct(), we were blindly setting the error into
> the caller's errp parameter.  But a future patch to split
> visit_end_struct() will require that we take action based
> on whether an error has occurred, which requires us to track
> all actions through a local err.  Rewrite the visits to be
> more in line with the other generated calls.
>
> Generated code changes look like:
>
> |@@ -42,12 +42,18 @@ void visit_type_GuestAgentCommandInfo(Vi

I'd drop this line.

> |     Error *err = NULL;
> |
> |     visit_start_struct(v, (void **)obj, "GuestAgentCommandInfo", name, 
> sizeof(GuestAgentCommandInfo), &err);
> |-    if (!err) {
> |-        if (*obj) {
> |-            visit_type_GuestAgentCommandInfo_fields(v, obj, errp);
> |-        }
> |-        visit_end_struct(v, &err);
> |+    if (err) {
> |+        goto out;
> |     }
> |+    if (!*obj) {

err is clear here.

> |+        goto out_obj;
> |+    }
> |+    visit_type_GuestAgentCommandInfo_fields(v, obj, &err);
> |+out_obj:
> |+    error_propagate(errp, err);
> |+    err = NULL;

If we come from goto out_obj, these two lines are no-ops.

> |+    visit_end_struct(v, &err);
> |+out:
> |     error_propagate(errp, err);
> | }
>
> Signed-off-by: Eric Blake <address@hidden>
> Reviewed-by: Marc-André Lureau <address@hidden>
>
> ---
> v9: enhance commit message
> v8: no change
> v7: place earlier in series
> v6: based loosely on v5 7/46, but mostly a rewrite to get the last
> generated code in the same form as all the others, so that the
> later conversion to split visit_check_struct() will be easier
> ---
>  scripts/qapi-visit.py | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index b93690b..4a4f67d 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -123,12 +123,18 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s 
> **obj, const char *name, Error
>      Error *err = NULL;
>
>      visit_start_struct(v, (void **)obj, "%(name)s", name, 
> sizeof(%(c_name)s), &err);
> -    if (!err) {
> -        if (*obj) {
> -            visit_type_%(c_name)s_fields(v, obj, errp);
> -        }
> -        visit_end_struct(v, &err);
> +    if (err) {
> +        goto out;
>      }
> +    if (!*obj) {
> +        goto out_obj;
> +    }
> +    visit_type_%(c_name)s_fields(v, obj, &err);
> +out_obj:
> +    error_propagate(errp, err);
> +    err = NULL;
> +    visit_end_struct(v, &err);
> +out:
>      error_propagate(errp, err);
>  }
>  ''',

gen_visit_union(), the other generator of visit_start_struct(), already
does it this way.  It generates additional goto out_obj, so the
placement of out_obj makes more sense there.  I guess we want to place
it in the same spot here to facilitate unifying the two.