Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com>
> ---
> 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.
