Re: Questionable aspects of QEMU Error's design

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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.

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


-- 
Best regards,
Vladimir