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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

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

I kept its scope as narrow as I could.  More on that below.

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

This is due to the structure of the code generator:

       for memb in members:
           ret += gen_if(memb.ifcond)
           if memb.optional:
               ret += mcgen('''
       if (visit_optional(v, "%(name)s", &obj->has_%(c_name)s)) {
   ''',
                            name=memb.name, c_name=c_name(memb.name))
               push_indent()
           ret += mcgen('''
       if (!visit_type_%(c_type)s(v, "%(name)s", &obj->%(c_name)s, errp)) {
           return false;
       }
   ''',
                        c_type=memb.type.c_name(), name=memb.name,
                        c_name=c_name(memb.name))
           if memb.optional:
               pop_indent()
               ret += mcgen('''
       }
   ''')
           ret += gen_endif(memb.ifcond)

Note how we splice in the if (visit_optional(...)) without affecting the
common part.

I doubt messing with it is worthwhile.

>> -
>> -    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;'?

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

Narrow scope: this patch does just enough to make the functions return
bool.  It freely uses such bool function values for this purpose, but it
refrains from exploiting them for more.  That's left to later patches,
in this case PATCH 25.

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

Also left for PATCH 25.

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

Likewise.

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

Generated by the code as visit_type_UserDefOne_members() above.

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

Will fix.

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

Will do.

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

ACK.

Thanks!