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
> 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

It is inconventient, and does not solve the problem, because of the race 
condition between
when the first function is called and then the second one is called.

> a simpler wrapper function that works for the majority of cases where a
> boolean result for the combined operation is enough.
> 
> Kevin
> 
>