Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one

[Claudio Fontana]

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.
 */

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