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

[Markus Armbruster]

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

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

By the way, the "one" in "module_load_one" & friends feels redundant.
When I see "module_load", I assume it loads one module.

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