Re: Questionable aspects of QEMU Error's design

[Markus Armbruster <armbru@redhat.com>]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 02.04.2020 11:55, Markus Armbruster wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On Thu, 2 Apr 2020 at 07:11, Vladimir Sementsov-Ogievskiy
>>> <vsementsov@virtuozzo.com> 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.