Re: Questionable aspects of QEMU Error's design

[Daniel P . Berrangé]

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 :|