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

[Kevin Wolf]

Am 22.09.2022 um 14:42 hat Markus Armbruster geschrieben:
> Claudio Fontana <cfontana@suse.de> writes:
> 
> > 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,
> 
> 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).

My intuition with this one was that "not found" is still an error case,
but it's an error case that we need to distinguish from other error
cases.

Is this one of the rare use cases for error classes?

Kevin