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

[Claudio Fontana]

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);