Re: Questionable aspects of QEMU Error's design

[Markus Armbruster]

Daniel P. Berrangé <address@hidden> writes:

> 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));

This assumes !defined(NDEBUG).

> or
>
>    if (!qemu_boot_set(boot_once, NULL))
>        abort()
>
> and would never have invented &error_fatal.

Yes, the readability improvement of &error_abort over this is only
marginal.

However, &error_abort also results in more useful stack backtraces, as
Vladimir already pointed out.

Our use of error_propagate() sabotages this advantage.  Vladimir's auto
propagation work stops that.

>> * 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.

It's a matter of patching the big comment in error.h.

Of course, the non-preferred style will still be copied from bad
examples until we get rid of them.

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

Manual conversion of a modest set of frequently used functions with
automated conversion of its calls should be feasible.

For more, I believe we need to figure out how to automatically transform
code that returns errors.

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

If I remember correctly, error.h predates our adoption of GLib.  Water
under the bridge now.