Re: Questionable aspects of QEMU Error's design

[Markus Armbruster <armbru@redhat.com>]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Thu, 2 Apr 2020, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>> 02.04.2020 12:36, BALATON Zoltan wrote:
[...]
>>>> Not much better. Could it be something like:
[...]
>>>>      ERRP_RET(object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>>>>                                        "memory", errp));
>>>>      ERRP_RET(object_property_set_link(cpuobj, OBJECT(s), "idau", errp));
>>>>      ERRP_RET(object_property_set_bool(cpuobj, true, "realized", errp));
>>>>
>>>
>>> and turn all
>>>
>>> ret = func(...);
>>> if (ret < 0) {
>>>     return ret;
>>> }
>>>
>>> into
>>>
>>> FAIL_RET(func(...))
>>>
>>> ?
>>>
>>> Not a problem to make such macro.. But I think it's a bad idea to turn all the code
>>> into sequence of macro invocations. It's hard to debug and follow.
>>
>> Yes.  Hiding control flow in macros is almost always too much magic.
>> There are exceptions, but this doesn't look like one.
>
> I did't like this idea of mine too much either so I agree but I see no
> other easy way to simplify this. If you propose changing function
> return values maybe these should return errp instead of passing it as
> a func parameter? Could that result in simpler code and less macro
> magic needed?

I don't think so.  Let's compare a few error handling patterns from
error.h.

 * Call a function and receive an error from it:
 *     Error *err = NULL;
 *     foo(arg, &err);
 *     if (err) {
 *         handle the error...
 *     }

Making foo() return the error object instead of void looks like an
obvious win:

       if (foo(arg)) {
           handle the error...
       }

Except it does not work, because surely a use of @err is hiding in
"handle the error" (or else we'd leak it).  So you actually need

       err = foo(arg)
       if (err) {
           handle the error...
       }

All this saves you is initializing @err.

You can save a bit more

       if ((err = foo(arg))) {
           handle the error...
       }

We generally frown upon assignemnts within if controlling expressions.

Next:

 * Receive an error and pass it on to the caller:
 [...]
 * But when all you do with the error is pass it on, please use
 *     foo(arg, errp);
 * for readability.

The obvious conversion

       *errp = foo(arg);

is wrong for !errp, errp == &error_fatal, and errp == &error_abort.
You'd need

       err = foo(arg);
       error_propagate(errp, err);

or, if you're so inclined

       error_propagate(errp, foo(arg));

Less legible, and it creates and destroys error objects where the
current method doesn't.

Returning the error object is even less attractive for functions that
now return values.