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

[Markus Armbruster]

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

> 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]

No, that one's generated from tests/qapi-schema/qapi-schema-test.json,
while this one is generated from example-schema.json.

>>   [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.

Eric spotted this in v1, and asked for an explanation.  Perhaps I should
have added a note to my commit message then.

This patch has a narrow scope: it does just enough to make the functions
return bool.  The result is a bit weird in places.  PATCH 39 will
straighten it out.

We can't drop the @err and error_propagate() here just yet, because the
generator can still generate code that needs it, just not for this
particular input.

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

Falcon eyes!  I'll fix it.

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

You're right.

qapi_clone_start_struct() only ever returns true, but relying on that
would be unclean.

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

Right again.

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

Thanks!