|
From: | BALATON Zoltan |
Subject: | Re: Questionable aspects of QEMU Error's design |
Date: | Thu, 2 Apr 2020 17:28:15 +0200 (CEST) |
User-agent: | Alpine 2.22 (BSF 395 2020-01-19) |
On Thu, 2 Apr 2020, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <address@hidden> 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 <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?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
[Prev in Thread] | Current Thread | [Next in Thread] |