On 9/22/22 11:21, Claudio Fontana wrote: > 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);
bool module_load_one(const char *prefix, const char *name, Error **errp, bool *found);