Re: Questionable aspects of QEMU Error's design

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: text/plain, Size: 4603 bytes --]

On Thu, 2 Apr 2020, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> 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 <armbru@redhat.com> 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.

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?

Regards,
BALATON Zoltan