Re: Questionable aspects of QEMU Error's design

[Vladimir Sementsov-Ogievskiy]

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.


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.


--
Best regards,
Vladimir