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

[Claudio Fontana]

On 9/22/22 14:33, Daniel P. Berrangé wrote:
> On Thu, Sep 22, 2022 at 02:30:35PM +0200, Claudio Fontana wrote:
>> On 9/22/22 12:37, Daniel P. Berrangé wrote:
>>> On Thu, Sep 22, 2022 at 11:34:22AM +0200, Claudio Fontana wrote:
>>>> On 9/22/22 11:31, Daniel P. Berrangé wrote:
>>>>> On Thu, Sep 22, 2022 at 11:20:07AM +0200, Claudio Fontana wrote:
>>>>>> On 9/22/22 10:28, Daniel P. Berrangé wrote:
>>>>>>>
>>>>>>>> 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).
>>>>>
>>>>> I suggested "try" as in the  g_malloc vs g_try_malloc API naming pattern,
>>>>> where the latter ignores the OOM error condition.
>>>>>
>>>>> So in this case 'try' means try to load the module, but don't fail if
>>>>> the module is missing on disk.
>>>>
>>>> I understand what you mean, but this is wrong in this case.
>>>>
>>>> We _do not fail_ in module_load_one, whether an error is present
>>>> or not, whether a module is found or not.
>>>
>>> Looking at the callers though, AFAIK there are only two patterns
>>> that we need. All callers should report a fatal error if the module
>>> exists and loading it failed eg module is from mis-matched build.
>>>
>>> Some callers also want a failure if the module doesn't exist on
>>
>> Some callers also want to behave differently if the module is not installed.
>> It is not a "failure" in the same sense that what an Error returns.
>>
>> For example, the block dmg module tries to also load other submodules for 
>> decompression.
>>
>> If dmg does not find any such submodules, it will be able to support basic 
>> dmg just fine,
>> but won't be able to open compressed dmg.
> 
> The dmg case looks like it works fine with module_try_load_one(). I
> know your patch does a "warn" when the module is missing currently,
> but IMHO that's the wrong place todo this. Any problems with use
> of compressed dmg should be reported at the time the feature is
> actually used, rather than when trying to load the module.

Right, this was the input from Kevin as well, I planned to change that.

We should warn only when we actually encounter/decode a compressed dmg.

Thanks,

Claudio