[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 07/46] qapi: Don't pass pre-existing error to
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v5 07/46] qapi: Don't pass pre-existing error to later call |
Date: |
Thu, 24 Sep 2015 10:14:37 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 |
On 09/24/2015 08:58 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, (continued)
[Qemu-devel] [PATCH v5 02/46] qapi: Clean up qapi.py per pep8, Eric Blake, 2015/09/21
[Qemu-devel] [PATCH v5 07/46] qapi: Don't pass pre-existing error to later call, Eric Blake, 2015/09/21
Re: [Qemu-devel] [PATCH v5 07/46] qapi: Don't pass pre-existing error to later call, Eric Blake, 2015/09/26
Re: [Qemu-devel] [PATCH v5 07/46] qapi: Don't pass pre-existing error to later call, Eric Blake, 2015/09/26
Re: [Qemu-devel] [PATCH v5 07/46] qapi: Don't pass pre-existing error to later call, Markus Armbruster, 2015/09/28
[Qemu-devel] [PATCH v5 08/46] qapi: Reuse code for flat union base validation, Eric Blake, 2015/09/21
[Qemu-devel] [PATCH v5 09/46] qapi: Use consistent generated code patterns, Eric Blake, 2015/09/21