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

[Claudio Fontana]

On 9/22/22 15:44, Daniel P. Berrangé wrote:
> On Thu, Sep 22, 2022 at 03:34:42PM +0200, Philippe Mathieu-Daudé wrote:
>> On Thu, Sep 22, 2022 at 3:20 PM Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>> Claudio Fontana <cfontana@suse.de> writes:
>>>
>>> [...]
>>>
>>>> I think it would be better to completely make the return value separate 
>>>> from the Error,
>>>> and really treat Error as an exception and not mix it up with the regular 
>>>> execution,
>>>>
>>>> but if it is the general consensus that I am the only one seeing this 
>>>> conflation problem we can model it this way too.
>>>
>>> It's a matter of language pragmatics.  In Java, you throw an exception
>>> on error.  In C, you return an error value.
>>>
>>> Trying to emulate exceptions in C might be even more unadvisable than
>>> trying to avoid them in Java.  Best to work with the language, not
>>> against it.
>>>
>>> Trouble is the error values we can conveniently return in C can't convey
>>> enough information.  So we use Error for that.  Just like GLib uses
>>> GError.
>>>
>>> More modern languages do "return error value" much better than C can.  C
>>> is what it is.
>>>
>>> We could certainly argue how to do better than we do now in QEMU's C
>>> code.  However, the Error API is used all over the place, which makes
>>> changing it expensive.  "Rethinking the whole Error API" (your words)
>>> would have to generate benefits worth this expense.  Which seems
>>> unlikely.
>>
>> QEMU Error* and GLib GError are designed to report recoverable runtime 
>> *errors*.
>>
>> There is or is no error. A boolean return value seems appropriate.
>>
>> We are bikeshedding about the API because we are abusing it in a non-error 
>> case.
>>
>> If we want to try to load an optional module, the Error* argument is
>> not the proper way to return the information regarding why we couldn't
>> load.
>>
>> In both cases we want to know if the module was loaded. If this is an
>> optional module, we don't care why it couldn't be loaded.
> 
> No, that's wrong. If the module exists on disk but is incompatible
> with the current QEMU, then we need to be reporting that as an
> error to the caller, so they can propagate this problem back up
> the stack to the QMP command or CLI arg that started the code path.

Agree.

> 
> We don't need to be using the return status to tell the caller if
> the module was loaded or not. We only should be telling thue caller
> is there was a reportable error or not.
> 
> Consider, there is a call to load block drivers. We don't need
> to know whether each block driver was loaded or not. eg if the
> 'curl' code is a module and we fail to load it, then when code
> tries to create a curl based block device the missing curl
> support will be reported at that time.  The callers that load
> modules should only need to express whether their load attempt
> is mandatory or optional, in terms of the module existing on
> disk.  If the modules exists on disk, any further errors
> encountered when loading it should be propagated.
> 
> 
> 
>> So trying to make everybody happy:
>>
>>   // Return true if the module could be loaded, otherwise return false
>> and errp contains the error.
>>  bool module_load_one(const char *prefix, const char *name, Error *errp);
>>
>>   // Return true if the module could be loaded, false otherwise.
>>   bool module_try_load_one(const char *prefix, const char *name);
> 
> Nope, this latter doesn't work as it throws away important errors
> when loading an incompatible/broken module.
> 

Agree.

Claudio