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

[Daniel P . Berrangé]

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.

> 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

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

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.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|