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

[Markus Armbruster]

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

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

[...]