Re: [PATCH 22/46] qapi: Make visitor functions taking Error ** return bool, not void

[Eric Blake]

On 6/24/20 11:43 AM, Markus Armbruster wrote:
> See recent commit "error: Document Error API usage rules" for
> rationale.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   docs/devel/qapi-code-gen.txt  |  51 +++++------
>   include/qapi/clone-visitor.h  |   8 +-
>   include/qapi/visitor-impl.h   |  26 +++---
>   include/qapi/visitor.h        | 102 ++++++++++++---------
>   audio/audio_legacy.c          |  15 ++--
>   qapi/opts-visitor.c           |  58 +++++++-----
>   qapi/qapi-clone-visitor.c     |  33 ++++---
>   qapi/qapi-dealloc-visitor.c   |  27 ++++--
>   qapi/qapi-visit-core.c        | 165 ++++++++++++++++++----------------
>   qapi/qobject-input-visitor.c  | 109 +++++++++++++---------
>   qapi/qobject-output-visitor.c |  27 ++++--
>   qapi/string-input-visitor.c   |  62 +++++++------
>   qapi/string-output-visitor.c  |  32 ++++---
>   scripts/qapi/visit.py         |  58 +++++-------
>   14 files changed, 435 insertions(+), 338 deletions(-)

Hefty, but I don't see a sane way to split it further.

> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index a7794ef658..9bfc57063c 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt

> -    void visit_type_UserDefOne_members(Visitor *v, UserDefOne *obj, Error 
> **errp)
> +    bool visit_type_UserDefOne_members(Visitor *v, UserDefOne *obj, Error 
> **errp)
>       {
>           Error *err = NULL;
>   
> -        visit_type_int(v, "integer", &obj->integer, &err);
> -        if (err) {
> -            goto out;
> +        if (!visit_type_int(v, "integer", &obj->integer, errp)) {
> +            return false;
>           }
>           if (visit_optional(v, "string", &obj->has_string)) {
> -            visit_type_str(v, "string", &obj->string, &err);
> -            if (err) {
> -                goto out;
> +            if (!visit_type_str(v, "string", &obj->string, errp)) {
> +                return false;
>               }
>           }

Is this worth compressing two 'if's into one:

if (visit_optional(...) &&
    !visit_type_str(...)) {
    return false;
}

> -
> -    out:
>           error_propagate(errp, err);
> +        return !err;

Now that 'err' is never anything but NULL, why aren't you dropping the 
error_propagate() and merely using 'return true;'?

>       }
>   
> -    void visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne **obj, Error **errp)
> +    bool visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne **obj, 
> Error **errp)
>       {
>           Error *err = NULL;
>   
> -        visit_start_struct(v, name, (void **)obj, sizeof(UserDefOne), &err);
> -        if (err) {
> -            goto out;
> +        if (!visit_start_struct(v, name, (void **)obj, sizeof(UserDefOne), 
> errp)) {
> +            return false;
>           }
>           if (!*obj) {
>               /* incomplete */
> @@ -1461,19 +1457,18 @@ Example:

Adding context:

>             assert(visit_is_dealloc(v));
>             goto out_obj;
>         }
>         visit_type_UserDefOne_members(v, *obj, &err);
>         if (err) {
>             goto out_obj;

Should this be:

> if (!visit_type_UserDefOne_members(v, *obj, &err)) {
>     goto out_obj;
>
>         }
>         visit_check_struct(v, &err);
>     out_obj:
>         visit_end_struct(v, (void **)obj);
>         if (err && visit_is_input(v)) {


>               qapi_free_UserDefOne(*obj);
>               *obj = NULL;
>           }
> -    out:
>           error_propagate(errp, err);
> +        return !err;

Here, err is still used by out_obj:, so this one is fine.

>       }
>   
> -    void visit_type_UserDefOneList(Visitor *v, const char *name, UserDefOneList **obj, Error **errp)
> +    bool visit_type_UserDefOneList(Visitor *v, const char *name, 
> UserDefOneList **obj, Error **errp)
>       {
>           Error *err = NULL;
>           UserDefOneList *tail;
>           size_t size = sizeof(**obj);
>   
> -        visit_start_list(v, name, (GenericList **)obj, size, &err);
> -        if (err) {
> -            goto out;
> +        if (!visit_start_list(v, name, (GenericList **)obj, size, errp)) {
> +            return false;
>           }
>   
>           for (tail = *obj; tail;
> @@ -1492,21 +1487,19 @@ Example:

Adding context:

>              tail = (UserDefOneList *)visit_next_list(v, (GenericList *)tail, 
> size)) {
>             visit_type_UserDefOne(v, NULL, &tail->value, &err);
>             if (err) {
>                 break;
>             }

Should this be:
if (visit_type_UserDefOne(...)) {
    break;

>         }
>
>         if (!err) {
>             visit_check_list(v, &err);
>         }
>         visit_end_list(v, (void **)obj);
>         if (err && visit_is_input(v)) {


>               qapi_free_UserDefOneList(*obj);
>               *obj = NULL;
>           }
> -    out:
>           error_propagate(errp, err);
> +        return !err;
>       }

Again, err is still used, so this one is fine.

>   
> -    void visit_type_q_obj_my_command_arg_members(Visitor *v, q_obj_my_command_arg *obj, Error **errp)
> +    bool visit_type_q_obj_my_command_arg_members(Visitor *v, 
> q_obj_my_command_arg *obj, Error **errp)
>       {
>           Error *err = NULL;
>   
> -        visit_type_UserDefOneList(v, "arg1", &obj->arg1, &err);
> -        if (err) {
> -            goto out;
> +        if (!visit_type_UserDefOneList(v, "arg1", &obj->arg1, errp)) {
> +            return false;
>           }
> -
> -    out:
>           error_propagate(errp, err);
> +        return !err;
>       }

But this is another one where err is unused.

>   
>   [Uninteresting stuff omitted...]
> diff --git a/include/qapi/clone-visitor.h b/include/qapi/clone-visitor.h
> index 5b665ee38c..adf9a788e2 100644
> --- a/include/qapi/clone-visitor.h

> +++ b/include/qapi/visitor.h
> @@ -286,6 +284,8 @@ void visit_free(Visitor *v);
>    * On failure, set *@obj to NULL and store an error through @errp.
>    * Can happen only when @v is an input visitor.
>    *
> + * Return true on succes, false on failure.

success

(copied several times in this file)

> +++ b/qapi/qapi-clone-visitor.c

> -static void qapi_clone_type_int64(Visitor *v, const char *name, int64_t *obj,
> +static bool qapi_clone_type_int64(Visitor *v, const char *name, int64_t *obj,
>                                      Error **errp)

Pre-existing indentation glitch that you could fix while here.

>   {
>       QapiCloneVisitor *qcv = to_qcv(v);
>   
>       assert(qcv->depth);
>       /* Value was already cloned by g_memdup() */
> +    return true;
>   }
>   
> -static void qapi_clone_type_uint64(Visitor *v, const char *name,
> +static bool qapi_clone_type_uint64(Visitor *v, const char *name,
>                                       uint64_t *obj, Error **errp)

Ditto for several more functions in this file.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org