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 11:34:22AM +0200, Claudio Fontana wrote:
> On 9/22/22 11:31, Daniel P. Berrangé wrote:
> > On Thu, Sep 22, 2022 at 11:20:07AM +0200, Claudio Fontana wrote:
> >> On 9/22/22 10:28, Daniel P. Berrangé wrote:
> >>>
> >>>> 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)
> >>
> >>
> >> Here we are murking again the normal behavior and the error path.
> >>
> >> What is the meaning of try? It's not as though we would error out inside 
> >> the function module_load_one,
> >> it's the _caller_ that needs to decide how to treat a return value of 
> >> found/not found, and the exception (Error).
> > 
> > I suggested "try" as in the  g_malloc vs g_try_malloc API naming pattern,
> > where the latter ignores the OOM error condition.
> > 
> > So in this case 'try' means try to load the module, but don't fail if
> > the module is missing on disk.
> 
> I understand what you mean, but this is wrong in this case.
> 
> We _do not fail_ in module_load_one, whether an error is present
> or not, whether a module is found or not.

Looking at the callers though, AFAIK there are only two patterns
that we need. All callers should report a fatal error if the module
exists and loading it failed eg module is from mis-matched build.

Some callers also want a failure if the module doesn't exist on
disk (module_load_one can be made todo this), but most callers
are happy to carry on if the module doesn't exist (module_try_load_one
can simply return success status if it doesn't exist).

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