Re: [PATCH v2 16/44] qapi: Make visitor functions taking Error ** return bool, not void

[Vladimir Sementsov-Ogievskiy]

02.07.2020 18:49, 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     |  67 ++++++++------
>   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, 452 insertions(+), 355 deletions(-)
>
> 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
> @@ -1408,42 +1408,38 @@ Example:
>       #include "example-qapi-types.h"
>   
>   
> -    void visit_type_UserDefOne_members(Visitor *v, UserDefOne *obj, Error **errp);
> -    void visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne **obj, 
> Error **errp);
> -    void visit_type_UserDefOneList(Visitor *v, const char *name, 
> UserDefOneList **obj, Error **errp);
> +    bool visit_type_UserDefOne_members(Visitor *v, UserDefOne *obj, Error 
> **errp);
> +    bool visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne **obj, 
> Error **errp);
> +    bool visit_type_UserDefOneList(Visitor *v, const char *name, 
> UserDefOneList **obj, Error **errp);
>   
> -    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);
>   
>       #endif /* EXAMPLE_QAPI_VISIT_H */
>       $ cat qapi-generated/example-qapi-visit.c

should it be tests/test-qapi-visit.c ? [preexisting, don't care]

>   [Uninteresting stuff omitted...]
>   
> -    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;
>               }
>           }
> -
> -    out:
>           error_propagate(errp, err);
> +        return !err;

Looks weird with redundant err variable.. Still works. I see, after the whole 
series it is handled, so, OK.

>       }
>   
> -    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;

[..]

> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index 5fe0276c1c..6d8f4b6928 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -133,7 +133,7 @@ opts_visitor_insert(GHashTable *unprocessed_opts, const 
> QemuOpt *opt)
>   }
>   
>   

[..]

>   
>   
> @@ -221,7 +224,7 @@ lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp)
>   }
>   
>   
> -static void
> +static bool
>   opts_start_list(Visitor *v, const char *name, GenericList **list, size_t size,
>                   Error **errp)
>   {
> @@ -238,6 +241,7 @@ opts_start_list(Visitor *v, const char *name, GenericList 
> **list, size_t size,
>       } else {
>           *list = NULL;

should return false here, as errp is set.


>       }
> +    return true;
>   }
>   
>   
> @@ -285,13 +289,14 @@ opts_next_list(Visitor *v, GenericList *tail, size_t size)
>   }
>   
>   

[..]

> diff --git a/qapi/qapi-clone-visitor.c b/qapi/qapi-clone-visitor.c
> index daab6819b4..5a54bd593f 100644
> --- a/qapi/qapi-clone-visitor.c
> +++ b/qapi/qapi-clone-visitor.c
> @@ -24,7 +24,7 @@ static QapiCloneVisitor *to_qcv(Visitor *v)
>       return container_of(v, QapiCloneVisitor, visitor);
>   }
>   
> -static void qapi_clone_start_struct(Visitor *v, const char *name, void **obj,
> +static bool qapi_clone_start_struct(Visitor *v, const char *name, void **obj,
>                                       size_t size, Error **errp)
>   {
>       QapiCloneVisitor *qcv = to_qcv(v);
> @@ -34,11 +34,12 @@ static void qapi_clone_start_struct(Visitor *v, const char 
> *name, void **obj,
>           /* Only possible when visiting an alternate's object
>            * branch. Nothing further to do here, since the earlier
>            * visit_start_alternate() already copied memory. */
> -        return;
> +        return true;
>       }
>   
>       *obj = g_memdup(*obj, size);
>       qcv->depth++;
> +    return true;
>   }
>   
>   static void qapi_clone_end(Visitor *v, void **obj)
> @@ -51,11 +52,12 @@ static void qapi_clone_end(Visitor *v, void **obj)
>       }
>   }
>   
> -static void qapi_clone_start_list(Visitor *v, const char *name,
> +static bool qapi_clone_start_list(Visitor *v, const char *name,
>                                     GenericList **listp, size_t size,
>                                     Error **errp)
>   {
>       qapi_clone_start_struct(v, name, (void **)listp, size, errp);
> +    return true;

should be return qapi_clone_start_struct(v, name, (void **)listp, size, errp);

>   }
>   
>   static GenericList *qapi_clone_next_list(Visitor *v, GenericList *tail,
> @@ -69,45 +71,49 @@ static GenericList *qapi_clone_next_list(Visitor *v, 
> GenericList *tail,
>       return tail->next;
>   }
>   
> -static void qapi_clone_start_alternate(Visitor *v, const char *name,
> +static bool qapi_clone_start_alternate(Visitor *v, const char *name,
>                                          GenericAlternate **obj, size_t size,
>                                          Error **errp)
>   {
>       qapi_clone_start_struct(v, name, (void **)obj, size, errp);
> +    return true;

similar here

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


With three return values fixed:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>



--
Best regards,
Vladimir