Re: Questionable aspects of QEMU Error's design

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <address@hidden> writes:

> 02.04.2020 12:36, BALATON Zoltan wrote:
>> On Thu, 2 Apr 2020, Vladimir Sementsov-Ogievskiy wrote:
>>> 01.04.2020 23:15, Peter Maydell wrote:
>>>> On Wed, 1 Apr 2020 at 10:03, Markus Armbruster <address@hidden> wrote:
>>>>>
>>>>> QEMU's Error was patterned after GLib's GError.  Differences include:
>>>>
>>>>  From my POV the major problem with Error as we have it today
>>>> is that it makes the simple process of writing code like
>>>> device realize functions horrifically boilerplate heavy;
>>>> for instance this is from hw/arm/armsse.c:
>>>>
>>>>          object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>>>>                                   "memory", &err);
>>>>          if (err) {
>>>>              error_propagate(errp, err);
>>>>              return;
>>>>          }
>>>>          object_property_set_link(cpuobj, OBJECT(s), "idau", &err);
>>>>          if (err) {
>>>>              error_propagate(errp, err);
>>>>              return;
>>>>          }
>>>>          object_property_set_bool(cpuobj, true, "realized", &err);
>>>>          if (err) {
>>>>              error_propagate(errp, err);
>>>>              return;
>>>>          }
>>>>
>>>> 16 lines of code just to set 2 properties on an object
>>>> and realize it. It's a lot of boilerplate and as
>>>> a result we frequently get it wrong or take shortcuts
>>>> (eg forgetting the error-handling entirely, calling
>>>> error_propagate just once for a whole sequence of
>>>> calls, taking the lazy approach and using err_abort
>>>> or err_fatal when we ought really to be propagating
>>>> an error, etc). I haven't looked at 'auto propagation'
>>>> yet, hopefully it will help?
>>>
>>> Yes, after it the code above will look like this:
>>>
>>> ... some_func(..., errp)
>>> {
>>>    ERRP_AUTO_PROPAGATE(); # magic macro at function start, and no "Error 
>>> *err" definition
>>>
>>> ...
>>>          object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>>>                                   "memory", errp);
>>>          if (*errp) {
>>>              return;
>>>          }
>>>          object_property_set_link(cpuobj, OBJECT(s), "idau", errp);
>>>          if (*errp) {
>>>              return;
>>>          }
>>>          object_property_set_bool(cpuobj, true, "realized", errp);
>>>          if (*errp) {
>>>              return;
>>>          }
>>> ...
>>> }
>>>
>>> - propagation is automatic, errp is used directly and may be safely 
>>> dereferenced.
>>
>> Not much better. Could it be something like:
>
> Actually, much better, as it solves some real problems around error 
> propagation.

The auto propagation patches' stated aim is to fix &error_fatal not to
eat hints, and to provide more useful stack backtraces with
&error_abort.  The slight shrinking of boilerplate is a welcome bonus.

For a bigger improvement, have the functions return a useful value, as
discussed elsewhere in this thread.

>>
>>      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.