Daniel P. Berrangé <berra...@redhat.com> writes:
> On Thu, Apr 02, 2020 at 07:54:11AM +0200, Markus Armbruster wrote:
>> Peter Maydell <peter.mayd...@linaro.org> writes:
>>
>> > On Wed, 1 Apr 2020 at 10:03, Markus Armbruster <arm...@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?
>>
>> 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
