Daniel P. Berrangé <berra...@redhat.com> 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.
