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));
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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
