On 9/22/22 11:38, Markus Armbruster wrote: > Daniel P. Berrangé <berra...@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