On 9/22/22 11:20, Claudio Fontana wrote: > On 9/22/22 10:28, Daniel P. Berrangé wrote: >> On Thu, Sep 22, 2022 at 08:07:43AM +0200, Markus Armbruster wrote: >>> Ease of use matters, too. When sticking to the rule leads to awkward >>> code, we should stop and think. Should we deviate from the rule? Or >>> can we avoid that by tweaking the interface? >>> >>> Philippe's proposed interface sticks to the rule. >> >> The cost is that when you see a function dosomething(true|false) as >> a reader you often have no idea what the effect of true vs false is >> on the behaviour of that function. You resort to looking at the >> API docs and/or code. This is where C would really benefit from >> having named parameters like as dosomething(ignore_errors=true|false) >> is totally obvious. Anyway, I digress. > > The confusion here I think stems from the fact that not finding a module is > _NORMAL BEHAVIOR_. > > We can configure the qemu package once including configuration for all > modules, > and then have the packager (or user) install the modules needed. > > We should break away from the easy-to-lean-to mindset that > > not finding a module => error path > > Because this is not the case. This is what is being confused in this > discussion. > > Distinguishing the normal execution path from the error path (exception, in > C++ parlance), > > we are just hindered by the fact that C can only have one return value. > > >> >>> 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). > > If this makes it clearer, lets keep the existing Error API pattern of using > both the return value and the Error parameter for the error (exception), > and put the NORMAL BEHAVIOR error value in an argument using a pointer. > > We do not pass a "bool ignore_errors" , because that is again confusing the > fact that it is not module_load_one that handles the errors, > module_load_one should neither handle nor ignore the errors, > it should generate an error in the error case, and a return value in the > normal case. > > What about: > > /* > > * module_load_one: attempt to load a module from a set of directories > > * > > * directories searched are: > > * - getenv("QEMU_MODULE_DIR") > > * - get_relocated_path(CONFIG_QEMU_MODDIR); > > * - /var/run/qemu/${version_dir} > > * > > * prefix: a subsystem prefix, or the empty string ("audio-", ..., > "") > * name: name of the module > > * errp: (ERROR CONDITION): errp will be set on module load error. > * found: (output): set to true if a module with this name has been > found, false if no such module is present. > * > > * Return value: true if no error encountered (module loaded or not > present). > * false if an error has been generated, and errp will be set > with the Error. > */ >
Now with the missing prototype: bool module_load_one(const char *prefix, const char *name, Error *errp, bool *found); > Thanks, > > C > > >> >> other names are available for the second, eg module_load_one_optional() >> >> Internally, both would call into a common helper following either >> Philippe's idea, or the -1/0/1 int return value. Either is fine, >> as they won't be exposed to any caller. >> >> With regards, >> Daniel >