On 9/22/22 19:05, Kevin Wolf wrote:
> Am 22.09.2022 um 17:27 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kw...@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
>
