[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] Error propagation in generated visitors and command mar
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] Error propagation in generated visitors and command marshallers |
Date: |
Fri, 11 Apr 2014 10:24:03 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) |
"Dr. David Alan Gilbert" <address@hidden> writes:
> * Markus Armbruster (address@hidden) wrote:
>> I stumbled over this while trying to purge error_is_set() from the code.
>
>> Here's how we commonly use the Error API:
>>
>> Error *err = NULL;
>>
>> foo(arg, &err)
>> if (err) {
>> goto out;
>> }
>> bar(arg, &err)
>> if (err) {
>> goto out;
>> }
>>
>> This ensures that err is null on entry, both for foo() and for bar().
>> Many functions rely on that, like this:
>>
>> void foo(ArgType arg, Error **errp)
>> {
>> if (frobnicate(arg) < 0) {
>> error_setg(errp, "Can't frobnicate");
>> // This asserts errp != NULL
>> }
>> }
>>
>>
>> Here's how some of our visitor code uses the Error API (for real code,
>> check out generated qmp-marshal.c):
>>
>> Error *err = NULL;
>> QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
>> Visitor *v = qmp_input_get_visitor(mi);
>> char *foo = NULL;
>> char *bar = NULL;
>>
>> visit_type_str(v, &foo, "foo", &err);
>> visit_type_str(v, &bar, "bar", &err);
>> if (err) {
>> goto out;
>> }
>>
>> Unlike above, this may pass a non-null errp to the second
>> visit_type_str(), namely when the first one fails.
>
> Right, one of the problems is you just have long strings of visit_* calls
> and adding a check to each one hides what you're actually doing in a sea
> of checks. The downside is that if one of those visit's fails then you've
> got no chance of figuring out which one it was.
>
> In my BER world I've got some macros along the lines of:
>
> #define LOCAL_ERR_REPORT(fallout) \
> if (local_err) { \
> fallout \
> }
>
> and at least then I can do things like:
> visit_type_str(v, &foo, "foo", &err);
> LOCAL_ERR_REPORT( goto out; )
> visit_type_str(v, &bar, "bar", &err);
> LOCAL_ERR_REPORT( goto out; )
>
> which while not nice,
Understatement :)
> means that you can actually follow the code, and
> I can also add a printf to the macro to record the function/line so
> that when one of them fails I can see which visit was the cause of the problem
> (something that's currently very difficult).
>
>> The visitor functions guard against that, like this:
>>
>> void visit_type_str(Visitor *v, char **obj, const char *name, Error
>> **errp)
>> {
>> if (!error_is_set(errp)) {
>> v->type_str(v, obj, name, errp);
>> }
>> }
>>
>> As discussed before, error_is_set() is almost almost wrong, fragile or
>> unclean. What if errp is null? Then we fail to stop visiting after an
>> error.
>>
>> The function could be improved like this:
>>
>> void visit_type_str(Visitor *v, char **obj, const char *name, Error
>> **errp)
>> {
>> assert(errp);
>> if (!*errp) {
>> v->type_str(v, obj, name, errp);
>> }
>> }
>>
>>
>> But: is it a good idea to have both patterns in the code? Should we
>> perhaps use the common pattern for visiting, too? Like this:
>>
>> visit_type_str(v, &foo, "foo", &err);
>> if (err) {
>> goto out;
>> }
>> visit_type_str(v, &bar, "bar", &err);
>> if (err) {
>> goto out;
>> }
>>
>> Then we can assume *errp is clear on function entry, like this:
>>
>> void visit_type_str(Visitor *v, char **obj, const char *name, Error
>> **errp)
>> {
>> v->type_str(v, obj, name, errp);
>> }
>>
>> Should execute roughly the same number of conditional branches.
>>
>> Tedious repetition of "if (err) goto out" in the caller, but that's what
>> we do elsewhere, and unlike elsewhere, these one's are generated.
>
> The other problem is I had a tendency to typo some of the cases to
> if (*err) and it's quite hard to spot and you wonder what's going on.
The only help I can offer with that is naming conventions: use "errp"
only for Error ** variables, and "err" only for Error *.
I have patches in my queue to clean up current usage.
- [Qemu-devel] Error propagation in generated visitors and command marshallers, Markus Armbruster, 2014/04/09
- Re: [Qemu-devel] Error propagation in generated visitors and command marshallers, Eric Blake, 2014/04/09
- Re: [Qemu-devel] Error propagation in generated visitors and command marshallers, Anthony Liguori, 2014/04/09
- Re: [Qemu-devel] Error propagation in generated visitors and command marshallers, Dr. David Alan Gilbert, 2014/04/09
- Re: [Qemu-devel] Error propagation in generated visitors and command marshallers,
Markus Armbruster <=
- Re: [Qemu-devel] Error propagation in generated visitors and command marshallers, Kevin Wolf, 2014/04/10
- Re: [Qemu-devel] Error propagation in generated visitors and command marshallers, Peter Crosthwaite, 2014/04/11