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 03:34:42PM +0200, Philippe Mathieu-Daudé wrote:
> On Thu, Sep 22, 2022 at 3:20 PM Markus Armbruster <armbru@redhat.com> wrote:
> >
> > Claudio Fontana <cfontana@suse.de> writes:
> >
> > [...]
> >
> > > I think it would be better to completely make the return value separate 
> > > from the Error,
> > > and really treat Error as an exception and not mix it up with the regular 
> > > execution,
> > >
> > > but if it is the general consensus that I am the only one seeing this 
> > > conflation problem we can model it this way too.
> >
> > It's a matter of language pragmatics.  In Java, you throw an exception
> > on error.  In C, you return an error value.
> >
> > Trying to emulate exceptions in C might be even more unadvisable than
> > trying to avoid them in Java.  Best to work with the language, not
> > against it.
> >
> > Trouble is the error values we can conveniently return in C can't convey
> > enough information.  So we use Error for that.  Just like GLib uses
> > GError.
> >
> > More modern languages do "return error value" much better than C can.  C
> > is what it is.
> >
> > We could certainly argue how to do better than we do now in QEMU's C
> > code.  However, the Error API is used all over the place, which makes
> > changing it expensive.  "Rethinking the whole Error API" (your words)
> > would have to generate benefits worth this expense.  Which seems
> > unlikely.
> 
> QEMU Error* and GLib GError are designed to report recoverable runtime 
> *errors*.
> 
> There is or is no error. A boolean return value seems appropriate.
> 
> We are bikeshedding about the API because we are abusing it in a non-error 
> case.
> 
> If we want to try to load an optional module, the Error* argument is
> not the proper way to return the information regarding why we couldn't
> load.
> 
> In both cases we want to know if the module was loaded. If this is an
> optional module, we don't care why it couldn't be loaded.

No, that's wrong. If the module exists on disk but is incompatible
with the current QEMU, then we need to be reporting that as an
error to the caller, so they can propagate this problem back up
the stack to the QMP command or CLI arg that started the code path.

We don't need to be using the return status to tell the caller if
the module was loaded or not. We only should be telling thue caller
is there was a reportable error or not.

Consider, there is a call to load block drivers. We don't need
to know whether each block driver was loaded or not. eg if the
'curl' code is a module and we fail to load it, then when code
tries to create a curl based block device the missing curl
support will be reported at that time.  The callers that load
modules should only need to express whether their load attempt
is mandatory or optional, in terms of the module existing on
disk.  If the modules exists on disk, any further errors
encountered when loading it should be propagated.



> So trying to make everybody happy:
> 
>   // Return true if the module could be loaded, otherwise return false
> and errp contains the error.
>  bool module_load_one(const char *prefix, const char *name, Error *errp);
> 
>   // Return true if the module could be loaded, false otherwise.
>   bool module_try_load_one(const char *prefix, const char *name);

Nope, this latter doesn't work as it throws away important errors
when loading an incompatible/broken 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 :|