On 9/22/22 15:44, Daniel P. Berrangé wrote: > 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 <arm...@redhat.com> wrote: >>> >>> Claudio Fontana <cfont...@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.
Agree. > > 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. > Agree. Claudio
