Re: Questionable aspects of QEMU Error's design

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <address@hidden> writes:

> 02.04.2020 11:55, Markus Armbruster wrote:
>> Peter Maydell <address@hidden> writes:
>>
>>> On Thu, 2 Apr 2020 at 07:11, Vladimir Sementsov-Ogievskiy
>>> <address@hidden> wrote:
>>>> Somehow, in general, especially with long function names and long 
>>>> parameter lists I prefer
>>>>
>>>> ret = func(..);
>>>> if (ret < 0) {
>>>>       return ret;
>>>> }
>>>
>>> Personally I prefer the other approach -- this one has an extra line
>>> in the source and
>>> needs an extra local variable.
>>
>> Me too, except when func(...) is so long that
>>
>>      if (func(...) < 0) {
>>
>> becomes illegible like
>>
>>      if (func(... yadda, yadda,
>>               yadda, ...) < 0) {
>>
>> Then an extra variable can improve things.
>>
>>>> Are you sure that adding a lot of boolean functions is a good idea? I 
>>>> somehow feel better with more usual int functions with -errno on failure.
>>>>
>>>> Bool is a good return value for functions which are boolean by nature: 
>>>> checks, is something correspond to some criteria. But for reporting an 
>>>> error I'd prefer -errno.
>>>
>>> When would we want to return an errno? I thought the whole point of the
>>> Error* was that that was where information about the error was returned.
>>> If all your callsites are just going to do "if (ret < 0) { ... } then having
>>> the functions pick an errno value to return is just extra work.
>>
>> 0/-1 vs. true/false is a matter of convention.  Lacking convention, it's
>> a matter of taste. >
>> 0/-1 vs. 0/-errno depends on the function and its callers.  When -errno
>> enables callers to distinguish failures in a sane and simple way, use
>> it.  When -errno feels "natural", I'd say feel free to use it even when
>> all existing callers only check < 0.
>>
>> When you return non-null/null or true/false on success/error, neglecting
>> to document that in a function contract can perhaps be tolerated; you're
>> using the return type the common, obvious way.  But when you return 0/-1
>> or 0/-errno, please spell it out.  I've seen too many "Operation not
>> permitted" that were actually -1 mistaken for -EPERM.  Also too many
>> functions that return -1 for some failures and -errno for others.
>>
>
> I just want to add one note:
>
> OK, you like the pattern
>
>   if (func()) {
>       <handle error>
>   }
>
> , I can live with it.
>
> I believe, we have a lot of such patterns already in code.
>
> Now, we are going to add a lot of functions, returning true on success and 
> false on failure, so add a lot of patterns
>
>   if (!func()) {
>       <handle error>
>   }

We already have this pattern all over the place with functions returning
non-null pointers on success, null pointer on failure.

> ---
>
> After it, looking at something like
>
>   if (!func()) {} / if (func()) {}
>
> I'll have to always jump to function definition, to check is it int or bool 
> function, to understand what exactly is meant and is there a mistake in the 
> code..
> So, I'm afraid that such conversion will not help reviewing/understanding the 
> code. I'd prefer to avoid using two opposite conventions in on project.

C sucks :)

Conventions help mitigate.  Here's one: when fun() returns
non-negative/negative on success/error, always use

    fun(...) < 0

or

    fun(...) >= 0

to check.  I dislike the latter.

When returns 0/negative, always use

    fun(...) < 0

Avoid

    fun(...) >= 0

because that suggests it could return a positive value, which is wrong.

Avoid

    fun(...)

because that requires the reader to know the return type.

When its returns non-null/null or true/false on success/failure, always
use

    !fun(...)

Avoid

    fun(...)

because that requires the reader to know the return type.

Note that we have a usable check for failure in all cases.  Goes well
with the habit to handle errors like this whenever practical

    if (failed) {
        handle failure
        bail out
    }
    handle success

> I can also imagine combining different function types (int/bool) in if 
> conditions o_O, what will save us from it?

Can you give an example?

> And don't forget about bool functions, which just check something, and false 
> is not an error, but just negative answer on some question.

For what it's worth, GLib specifically advises against such function
contracts:

    By convention, if you return a boolean value indicating success then
    TRUE means success and FALSE means failure.  Avoid creating
    functions which have a boolean return value and a GError parameter,
    but where the boolean does something other than signal whether the
    GError is set.  Among other problems, it requires C callers to
    allocate a temporary error.  Instead, provide a "gboolean *" out
    parameter.  There are functions in GLib itself such as
    g_key_file_has_key() that are deprecated because of this.