Re: Questionable aspects of QEMU Error's design

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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