Re: Questionable aspects of QEMU Error's design

[Alex Bennée]

Daniel P. Berrangé <address@hidden> writes:

> On Thu, Apr 02, 2020 at 07:54:11AM +0200, Markus Armbruster wrote:
>> Peter Maydell <address@hidden> writes:
>> 
>> > 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?
>> 
>> With that, you can have
>> 
>>         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;
>>         }
>> 
>> but you have to add
>> 
>>         ERRP_AUTO_PROPAGATE();
>> 
>> right at the beginning of the function.
>> 
>> It's a small improvement.  A bigger one is
>> 
>>         if (object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>>                                      "memory", errp)) {
>>             return;
>>         }
>>         if (object_property_set_link(cpuobj, OBJECT(s), "idau", errp)) {
>>             return;
>>         }
>>         if (object_property_set_bool(cpuobj, true, "realized", errp)) {
>>             return;
>>         }
>> 
>> This is item "Return value conventions" in the message you replied to.
>
> Even better, we can then string the checks together
>
>         if (object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>                                       "memory", errp) ||
>             object_property_set_link(cpuobj, OBJECT(s), "idau", errp) ||
>             object_property_set_bool(cpuobj, true, "realized", errp)) {
>              return;
>         }

You know at this point I wonder if we can't come up with some data table
that describes all these object interactions and a helper function that
processes it and tells us if it worked or not?

We are essentially just filling out an data structure anyway with all
this stuff.

>  
> Regards,
> Daniel


-- 
Alex Bennée