Re: [Qemu-devel] Error propagation in generated visitors and command marshallers

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 09.04.2014 um 17:48 hat Markus Armbruster geschrieben:
>> 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.
>> 
>> 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.
>> 
>> Opinions?
>
> I agree, use the same style as everywhere else.
>
> The pattern in the generated visitor that I find more annoying, though,
> is that it has a lot of code like:
>
>     if (!error_is_set(errp)) {
>         /* long block of code here */
>     }
>
> And I believe there are even cases where this nests.

I also find "if (error) bail_out" generally more readable than "if
(!error) do_more_work".  More so when nested.

I'll see what I can do about it in the generator scripts.

>                                                      There are also
> error_propagate() calls that can (and do in the common case) propagate
> NULL, this way selecting the first error, if any, but not stopping on
> the first error. I always found it confusing to read that code.

Can you point me to an instance in the generated code?