[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: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v5 07/46] qapi: Don't pass pre-existing error to later call |
Date: |
Mon, 28 Sep 2015 11:14:25 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 09/24/2015 10:14 AM, Eric Blake wrote:
>
>>>>
>>>> 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.
>
> Oh well. We do have a compelling use: qmp-input-visitor.c can set errp
> during visit_end_struct() when in strict mode (basically, the mode which
> warns if the input QDict has leftover key:value pairs that were not
> consumed by visit_type_FOO() between the start/end call). I don't know
> if visit_end_list() or visit_end_implicit_struct() care; but then we
> have the argument that it is worth keeping them consistent with
> visit_end_struct() which can indeed raise an error.
>
> One other potential alternative: What if we split visit_end_struct()
> into two visitor functions, one that checks for success, and the other
> that is called unconditionally to clean up resources. That is, go from:
>
> visit_start_struct(m, (void **)obj, "foo", name, sizeof(FOO), &err);
> if (!err) {
> if (*obj) {
> visit_type_FOO_fields(m, obj, errp);
> }
> visit_end_struct(m, &err);
> }
> error_propagate(errp, err);
>
> to a form where the check for leftover key/value pairs is only done on
> success with a new visit_check_struct():
>
> visit_start_struct(m, (void **)obj, "foo", name, sizeof(FOO), &err);
> if (!err) {
> if (*obj) {
> visit_type_FOO_fields(m, obj, &err);
> }
> if (!err) {
> visit_check_struct(m, &err);
> }
> visit_end_struct(m);
> }
> error_propagate(errp, err);
I think this split could help with writing safe code: in
visit_check_struct() you can rely on "no error so far", as usual. In
visit_end_struct(), you can't, but it should be a pure cleanup function,
where that's quite normal.
Looks like we're getting drawn into visitor contract territory again.
- 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
[Qemu-devel] [PATCH v5 10/46] qapi: Merge generation of per-member visits, Eric Blake, 2015/09/21