Re: [Qemu-devel] [PATCH v5 07/46] qapi: Don't pass pre-existing error to later call

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Due to the existing semantics of the error_set() family,
> generated sequences in the qapi visitors such as:
>
>     visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err);
>         if (!err) {
>             visit_type_FOO_fields(m, obj, errp);
>             visit_end_implicit_struct(m, &err);
>         }
>     error_propagate(errp, err);

Indentation seems off.  Intentional?

>
> are risky: if visit_type_FOO_fields() sets errp, and then
> visit_end_implicit_struct() also encounters an error, the
> attempt to overwrite the first error will cause an abort().
> Obviously, we weren't triggering this situation (none of the
> existing callbacks for visit_end_implicit_struct() currently
> try to set an error), but it is better to not even cause the
> problem in the first place.

The code works, but it sets a problematic example.

> Meanwhile, in spite of the poor documentation of the qapi
> visitors, we want to guarantee that if a visit_start_*()
> succeeds, then the matching visit_end_*() will be called.

Agreed.

> The options are to either propagate and clear a local err
> multiple times:
>
>     visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err);
>         if (!err) {
>             visit_type_FOO_fields(m, obj, &err);
>             if (err) {
>                 error_propagate(errp, err);
>                 err = NULL;
>             }
>             visit_end_implicit_struct(m, &err);
>         }
>     error_propagate(errp, err);
>
> or, as this patch does, just pass in NULL to ignore further
> errors once an error has occurred.
>
>     visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err);
>         if (!err) {
>             visit_type_FOO_fields(m, obj, &err);
>             visit_end_implicit_struct(m, err ? NULL : &err);
>         }
>     error_propagate(errp, err);

Hmmmmm... not sure we do this anywhere else, yet.  The ternary isn't
exactly pretty, but the intent to ignore additional error is clear
enough, I think.

If we elect to adopt this new error handling pattern, we should perhaps
document it in error.h.

Third option: drop visit_end_implicit_struct()'s errp parameter.  If we
find a compelling use for it, we'll put it back and solve the problem.

>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  scripts/qapi-visit.py | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 97343cf..d911b20 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -51,8 +51,8 @@ static void visit_type_implicit_%(c_type)s(Visitor *m, 
> %(c_type)s **obj, Error *
>
>      visit_start_implicit_struct(m, (void **)obj, sizeof(%(c_type)s), &err);
>      if (!err) {
> -        visit_type_%(c_type)s_fields(m, obj, errp);
> -        visit_end_implicit_struct(m, &err);
> +        visit_type_%(c_type)s_fields(m, obj, &err);
> +        visit_end_implicit_struct(m, err ? NULL : &err);
>      }
>      error_propagate(errp, err);
>  }
> @@ -143,9 +143,9 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, 
> const char *name, Error
>      visit_start_struct(m, (void **)obj, "%(name)s", name, 
> sizeof(%(c_name)s), &err);
>      if (!err) {
>          if (*obj) {
> -            visit_type_%(c_name)s_fields(m, obj, errp);
> +            visit_type_%(c_name)s_fields(m, obj, &err);
>          }
> -        visit_end_struct(m, &err);
> +        visit_end_struct(m, err ? NULL : &err);
>      }
>      error_propagate(errp, err);
>  }

Oh, it's about visit_end_struct(), too.  Commit message only talks about
visit_end_implicit_struct().

In particular, "none of the existing callbacks for
visit_end_implicit_struct() currently try to set an error".  Does that
hold for visit_end_struct() callbacks, too?

> @@ -175,9 +175,7 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, 
> const char *name, Error
>          visit_type_%(c_elt_type)s(m, &native_i->value, NULL, &err);
>      }
>
> -    error_propagate(errp, err);
> -    err = NULL;
> -    visit_end_list(m, &err);
> +    visit_end_list(m, err ? NULL : &err);
>  out:
>      error_propagate(errp, err);
>  }

Likewise.  Does it hold for visit_end_list() callbacks, too?

Looks like you switch from option 1 to option 2 here.  Your slate isn't
as clean as the commit message suggests :)

> @@ -231,9 +229,7 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, 
> const char *name, Error
>          abort();
>      }
>  out_end:
> -    error_propagate(errp, err);
> -    err = NULL;
> -    visit_end_implicit_struct(m, &err);
> +    visit_end_implicit_struct(m, err ? NULL : &err);
>  out:
>      error_propagate(errp, err);
>  }

Looks like another switch from option 1 to option 2.

> @@ -330,10 +326,8 @@ out_obj:
>          error_propagate(errp, err);
>          err = NULL;
>          visit_end_union(m, !!(*obj)->data, &err);
> -        error_propagate(errp, err);
> -        err = NULL;
>      }
> -    visit_end_struct(m, &err);
> +    visit_end_struct(m, err ? NULL : &err);
>  out:
>      error_propagate(errp, err);
>  }

Again.