Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one

[Claudio Fontana]

On 9/22/22 17:27, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 22.09.2022 um 14:42 hat Markus Armbruster geschrieben:
> 
> [...]
> 
>>> If you have callers that need to distinguish between not found, found
>>> but bad, found and good, then return three distinct values.
>>>
>>> I proposed to return -1 for found but bad (error), 0 for not found (no
>>> error), and 1 for loaded (no error).
>>
>> My intuition with this one was that "not found" is still an error case,
>> but it's an error case that we need to distinguish from other error
>> cases.
>>
>> Is this one of the rare use cases for error classes?
> 
> If I remember correctly, "not found" is not actually an error for most
> callers.  If we make it one, these callers have to error_free().  Minor
> annoyance, especially when you have to add an else just for that.

That's because the "Error" class, as implemented in QEMU, (and possibly in 
Glib..) seems to be closer to an Exception than an Error,
just like in C++.

And like in C++, the Exception is a more costly code path, that should not 
carry the regular runtime behavior,
it should really be representing only the "Exceptional" runtime case.

And this matches this specific instance perfectly.

Not finding the module should not raise an exception, because depending on the 
packaging the "module not present" is not an exceptional runtime behavior at 
all.

> 
> Even if we decide to make it an error, I would not create an error class
> for it.  I like
> 
>     rc = module_load_one(..., errp);
>     if (rc == -ENOENT) {
>         error_free(*errp);
>     } else if (rc < 0) {
>         return;
>     }
> 
> better than
> 
>     Error *err = NULL;
> 
>     module_load_one(..., &err);
>     if (err && err->class == ERROR_CLASS_NOT_FOUND) {
>         error_free(err);
>     } else if (err) {
>         error_propagate(errp, err);
>         return;
>     }
> 
> I respect your intuition.  Would it still say "error case" when the
> function is called module_load_if_exists()?
> 
> Hmm, another thought... a need to distinguish error cases can be a
> symptom of trying to do too much in one function.  We could split this
> into "look up module" and "actually load it".
>