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

[Claudio Fontana]

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

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

> 
>> Internally, both would call into a common helper following either
>> Philippe's idea, or the -1/0/1 int return value. Either is fine,
>> as they won't be exposed to any caller.
> 
> Yup.
> 

C