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

[Claudio Fontana <cfontana@suse.de>]

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
>