Re: Questionable aspects of QEMU Error's design

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

01.04.2020 15:10, Vladimir Sementsov-Ogievskiy wrote:
> 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.
> 


Or, maybe, go the hard way, add a script, which smartly search for error message in code-base,
correctly handling "%s" and multi-line messages.. And add to checkpatch a check
that new added error messages doesn't match to any of existing.

Hmm. I think, I'll try to do something like this.



-- 
Best regards,
Vladimir