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

[Claudio Fontana]

On 9/22/22 14:42, Markus Armbruster wrote:
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> On 9/22/22 11:38, Markus Armbruster wrote:
>>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>>
>>>> On Thu, Sep 22, 2022 at 08:07:43AM +0200, Markus Armbruster wrote:
>>>>> Ease of use matters, too.  When sticking to the rule leads to awkward
>>>>> code, we should stop and think.  Should we deviate from the rule?  Or
>>>>> can we avoid that by tweaking the interface?
>>>>>
>>>>> Philippe's proposed interface sticks to the rule.
>>>>
>>>> The cost is that when you see a  function   dosomething(true|false) as
>>>> a reader you often have no idea what the effect of true vs false is
>>>> on the behaviour of that function. You resort to looking at the
>>>> API docs and/or code.  This is where C would really benefit from
>>>> having named parameters like as  dosomething(ignore_errors=true|false)
>>>> is totally obvious. Anyway, I digress.
>>>
>>> Right.  Quoting myself: "If having to pass a flag turns out to to be a
>>> legibility issue, we can have wrapper functions."  :)
>>
>> There is something more fundamental that seems to be missed by most in this 
>> conversation,
>> ie the distinction between the normal execution path and the error path.
>>
>>
>>>
>>>>> Another interface that does: return -1 for error, 0 for module not found
>>>>> (no error), and 1 for loaded.
>>>>
>>>> IMHO this pattern is generally easier to understand when looking at
>>>> the callers, as the fatal error scenario is always clear.
>>>>
>>>> That said I would suggest neither approach as the public facing
>>>> API. Rather stop trying to overload 3 states onto an error reporting
>>>> pattern that inherantly wants to be 2 states. Instead just have
>>>> distinct methods
>>>
>>> Like these:
>>>
>>>>   bool module_load_one(const char *prefix, const char *name, Error *errp)
>>>>   bool module_try_load_one(const char *prefix, const char *name, Error 
>>>> *errp)
>>>>
>>>> other names are available for the second, eg module_load_one_optional()
>>>
>>> module_load_one_if_there()?
>>
>> And what do you do with the caller that needs to _know_ whether the module 
>> was "there" or not?
>>
>> This is losing this information along the way, and the callers NEED it.
>>
>> I really invite, with no offense intended,
> 
> None taken!
> 
>>                                            to read the hunks of my patch and 
>> the callers,
>> there are occasions where we need to _know_ if the module was there or not, 
>> and act depending on the context.
>>
>> The information about "bool is_there" needs to be passed to the caller.
> 
> 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).

That is fine too.

I think it would be better to completely make the return value separate from 
the Error,
and really treat Error as an exception and not mix it up with the regular 
execution,

but if it is the general consensus that I am the only one seeing this 
conflation problem we can model it this way too.

> 
>>> By the way, the "one" in "module_load_one" & friends feels redundant.
>>> When I see "module_load", I assume it loads one module.
>>
>> there is a module_load_all.
> 
> Libc has fcloseall() and fclose().  Clear enough, isn't it?
> 
> [...]
>