01.04.2020 15:44, Daniel P. Berrangé wrote:
On Wed, Apr 01, 2020 at 11:02:11AM +0200, Markus Armbruster wrote:
QEMU's Error was patterned after GLib's GError. Differences include:
* &error_fatal, &error_abort for convenience
I think this doesn't really need to exist, and is an artifact
of the later point "return values" where we commonly make methds
return void. If we adopted a non-void return value, then these
are no longer so compelling.
Consider if we didn't have &error_fatal right now, then we would
need to
Error *local_err = NULL;
qemu_boot_set(boot_once, &local_err)
if (*local_err)
abort();
This is tedious, so we invented &error_abort to make our lives
better
qemu_boot_set(boot_once, &error_abort)
If we had a "bool" return value though, we would probably have just
ended up doing:
assert(qemu_boot_set(boot_once, NULL));
But error_abort is better: it crashes where error fired and backtrace
in core file is backtrace of and error.
or
if (!qemu_boot_set(boot_once, NULL))
abort()
and would never have invented &error_fatal.
* Distinguishing different errors
Where Error has ErrorClass, GError has Gquark domain, gint code. Use
of ErrorClass other than ERROR_CLASS_GENERIC_ERROR is strongly
discouraged. When we need callers to distinguish errors, we return
suitable error codes separately.
The GQuark is just a static string, and in most cases this ends up being
defined per-file, or sometimes per functional group. So essentially you
can consider it to approximately a source file in most cases. The code
is a constant of some arbitrary type that is generally considered to be
scoped within the context of the GQuark domain.
* Return value conventions
Common: non-void functions return a distinct error value on failure
when such a value can be defined. Patterns:
- Functions returning non-null pointers on success return null pointer
on failure.
- Functions returning non-negative integers on success return a
negative error code on failure.
Different: GLib discourages void functions, because these lead to
awkward error checking code. We have tons of them, and tons of
awkward error checking code:
Error *err = NULL;
frobnicate(arg, &err);
if (err) {
... recover ...
error_propagate(errp, err);
}
Yeah, I really dislike this verbose style...
instead of
if (!frobnicate(arg, errp))
... recover ...
}
...so I've followed this style for any code I've written in QEMU
where possible.
Can also lead to pointless creation of Error objects.
I consider this a design mistake. Can we still fix it? We have more
than 2000 void functions taking an Error ** parameter...
Even if we don't do full conversion, we can at least encourage the
simpler style - previously reviewers have told me to rewrite code
to use the more verbose style, which I resisted. So at the very
least setting the expectations for preferred style is useful.
Transforming code that receives and checks for errors with Coccinelle
shouldn't be hard. Transforming code that returns errors seems more
difficult. We need to transform explicit and implicit return to
either return true or return false, depending on what we did to the
@errp parameter on the way to the return. Hmm.
Even if we only converted methods which are currently void, that
would be a notable benefit I think.
It is a shame we didn't just use GError from the start, but I guess
its probably too late to consider changing that now.
Regards,
Daniel
--
Best regards,
Vladimir
