Re: [Qemu-devel] [PATCH v5 07/46] qapi: Don't pass pre-existing error to later call

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5004 bytes --]

On 09/24/2015 08:58 AM, Markus Armbruster wrote:
> Eric Blake <eblake@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?

No, probably due to rebase churn (I reindented generated code in 9/46,
but the series as I worked on it wasn't always in the order presented
here).  Will fix.

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

More poor indentation on my part.

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

Ooh, interesting idea.  It changes the contract - but since the contract
isn't (yet) documented, and happens to work with existing uses without a
contract, it could indeed be nicer.  It would have knock-on effects to
24/46 where I first try documenting the contract.

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

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

I'm fairly certain that ALL of the visit_end_* callbacks were similar in
nature, but you've prompted me to re-audit things and update the commit
message to be absolutely clear about it.

> 
>> @@ -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 :)

Consistency is nice, but documenting where we started from to get to the
consistent state would be even nicer. Point taken.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]