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

[Claudio Fontana]

On 9/22/22 19:05, Kevin Wolf wrote:
> Am 22.09.2022 um 17:27 hat Markus Armbruster geschrieben:
>> 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.
> 
> AIUI most callers don't actually need the three way distinction, but

Right, this is why I initially modeled this as:

return value true -> found and loaded
return value false -> not found or error: (check *errp != NULL) to distinguish 
between the two.


> just success (may or may not be loaded now) and error.
> 
> The example for a caller that needs it was dmg. But with the changes
> after review, it won't be using the return code for this any more
> either.
> 
>> 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;
>>     }
> 
> That's a good point, we can use the return code to distinguish the cases.
> 
>> I respect your intuition.  Would it still say "error case" when the
>> function is called module_load_if_exists()?
> 
> I guess no, then it becomes a second success case.
> 
>> 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".
> 
> Might become slightly inconvenient. On the other hand, we can still have
> a simpler wrapper function that works for the majority of cases where a
> boolean result for the combined operation is enough.
> 
> Kevin
>