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 02:30:35PM +0200, Claudio Fontana wrote:
> On 9/22/22 12:37, Daniel P. Berrangé wrote:
> > 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
> 
> Some callers also want to behave differently if the module is not installed.
> It is not a "failure" in the same sense that what an Error returns.
> 
> For example, the block dmg module tries to also load other submodules for 
> decompression.
> 
> If dmg does not find any such submodules, it will be able to support basic 
> dmg just fine,
> but won't be able to open compressed dmg.

The dmg case looks like it works fine with module_try_load_one(). I
know your patch does a "warn" when the module is missing currently,
but IMHO that's the wrong place todo this. Any problems with use
of compressed dmg should be reported at the time the feature is
actually used, rather than when trying to load the module.


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