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

[Claudio Fontana]

On 9/22/22 16:36, Markus Armbruster wrote:
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> On 9/22/22 15:20, Markus Armbruster 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
>>
>> Right, we use Error for that and that's fine, but we should use it _only 
>> Error_ for that.
>>
>> Ie map the Exceptions directly to Error.
>>
>> So:
>>
>> try {
>>
>>   rv = function_call(...);
>>
>>   use_return_value(rv);
>>
>> } catch (Exception e) {
>>
>>   /* handle exceptional case */
>>
>> }
>>
>> becomes
>>
>> rv = function_call(..., Error **errp);
>>
>> if (errp) {
>>   /* handle exceptional case */
>> }
>>
>> use_return_value(rv);
> 
> This is simply not the intended use of Error.
> 
> Also, "trying to emulate exceptions in C might be even more unadvisable
> than trying to avoid them in Java."
> 
>> Instead we mix up the Exception code path and the regular code path, by 
>> having rv carry a mix of the Exception and regular return value,
>> and this creates problems and confusion.
> 
> "In C, you return an error value."
> 
>> If we put a hard line between the two, I think more clarity emerges.
> 
> Maybe, but consistency matters.  Clarity doesn't emerge in isolation.  
> Deviating from prevailing usage tends to confuse.
> 
>>> 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.
>>>
>>> [...]
>>>
>>
>> This is all fine, the problem is how we remodel this in C.
>>
>> This is how I see the next steps to clarify my position:
>>
>> short term:
>>
>> - keep the existing API with the existing assumptions, use a separate 
>> argument to carry the pointer to the actual return value (where the function 
>> return value as provided by the language is used to return if an exception 
>> was generated or not, as we assume today).
> 
> We don't actually need separate values for "actual return value" and
> "success vs. failure" here.  We can easily encode them in a single

Yes, we do, it would avoid the confusion that we see as soon as people look at 
the module_load_one code the first time.

> return value.  This is *common* in C, for better or worse.

We make our own life difficult, by wasting the very precious space of the 
return value that should be used to provide the meaning of the function,

and using it to provide a completely useless and redundant bool return value, 
that by your own definition,
is just "errp != NULL".

The very precious and scarce return value of the C function is completely 
wasted.

> 
> For instance, read() returns -1 on error, 0 on EOF (which is not an
> error), and a positive value on actual read.  On error, @errno is set,
> which is a bit awkward (we wouldn't design that way today, I hope).
> 
> The interface I proposed is similar: return -1 on error, 0 on not found
> (which is not an error), and 1 on successful load.  On error, an Error
> is set via @errp.  With a name that makes it obvious that "not found" is
> not an error, there's nothing that should surprise someone
> passingly familiar with QEMU code.

This is fine too, we can do -1 on error, 0 on not found and 1 (and errp set) on 
Error,

especially if the long term goal of actually fixing the high level problem in 
the Error API (separating it from the return value, leaving it free for 
meaningful return values for the ordinary case)
is not agreed on.

> 
>> - long term (maybe): fix the existing API by detaching completely the return 
>> value from the exception.
> 
> As I wrote, this seems unlikely to be worth its considerable expense.

In this case, something like your suggestion would be the second best option in 
my view.

C