Re: Questionable aspects of QEMU Error's design

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

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:

     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));

Regards,
BALATON Zoltan