Claudio Fontana <cfont...@suse.de> writes: > 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,
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? [...]
