Re: Questionable aspects of QEMU Error's design

["Alex Bennée" <alex.bennee@linaro.org>]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 01.04.2020 12:02, Markus Armbruster wrote:
>> QEMU's Error was patterned after GLib's GError.  Differences include:
>> * &error_fatal, &error_abort for convenience
>> * Error can optionally store hints
>> * Pointlessly different names: error_prepend() vs. g_error_prefix()
>> and
>>    so forth *shrug*
>> * Propagating errors
>>    Thanks to Vladimir, we'll soon have "auto propagation", which is
>> less
>>    verbose and less error-prone.
>> * Accumulating errors
>>    error_propagate() permits it, g_propagate_error() does not[*].
>>    I believe this feature is used rarely.  Perhaps we'd be better
>> off
>>    without it.  The problem is identifying its uses.  If I remember
>>    correctly, Vladimir struggled with that for his "auto propagation"
>>    work.
>>    Perhaps "auto propagation" will reduce the number of manual
>>    error_propagate() to the point where we can identify accumulations.
>>    Removing the feature would become feasible then.
>> * 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.
>> * 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);
>>      }
>>    instead of
>>      if (!frobnicate(arg, errp))
>>          ... recover ...
>>      }
>>    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...
>>    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.
>> 
>> [*] According to documentation; the code merely calls g_warning() then,
>> in typical GLib fashion.
>> 
>
>
> Side question:
>
> Can we somehow implement a possibility to reliably identify file and line number
> where error is set by error message?
>
> It's where debug of error-bugs always starts: try to imagine which parts of the error
> message are "%s", and how to grep for it in the code, keeping in mind also,
> that error massage may be split into several lines..
>
> Put file:line into each error? Seems too noisy for users.. A lot of errors are not
> bugs: use do something wrong and see the error, and understands what he is doing
> wrong.. It's not usual practice to print file:line into each message
> for user.

I tend to use __func__ for these things as the result is usually easily
grep-able.


>
>
> But what if we do some kind of mapping file:line <-> error code, so user will see
> something like:
>
>
>    Error 12345: Device drive-scsi0-0-0-0 is not found
>
> ....
>
> Hmm, maybe, just add one more argument to error_setg:
>
> error_setg(errp, 12345, "Device %s is not found", device_name);
>
> - it's enough grep-able.


-- 
Alex Bennée