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