[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Questionable aspects of QEMU Error's design
From: |
Markus Armbruster |
Subject: |
Re: Questionable aspects of QEMU Error's design |
Date: |
Fri, 03 Apr 2020 09:48:17 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Vladimir Sementsov-Ogievskiy <address@hidden> writes:
> 02.04.2020 18:06, Markus Armbruster wrote:
>> 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.
>
> Functions returning pointer are simpler to distinguish by name.
>
> Hmm, strange. But this pattern lose the pointer.. You mean
>
> ptr = func();
> if (!ptr) {
> <handle error>
> }
Yes, when you actually need the pointer. Common, but not universal.
> this is much more understandable. Usually ptr variable name and function name
> - all will help to understand that it's all about pointer.
>
>
>>
>>> ---
>>>
>>> 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
>
> This reduces chances that it fit in one line..
Yes, that's a drawback.
> But yes, if all use this convention, it makes it obvious what happening.
>
>>
>> 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.
>
> Exactly the problem I mention. To follow your suggestion, we'll have to update
> the whole code base.. However, why not.
Only if we value the consistency more than the update labor and churn.
>>
>> 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
>>
>
> OK. If convert everything to your suggestion it looks good.
>
> The only possible problem is boolean functions, which just check something,
> not returning the error..
>
> With a function like is_x_in_set(x, set), it's native to write
>
> if (is_x_in_set(x, set)) {
>
> ...
>
> }
>
> which is a bit in conflict with your suggestion. Still, such functions should
> be simply distinguished by name.
The conventions I described only apply to error checking. I certainly
don't mean to discourage things like
while (*p && qemu_isspace(*p)) {
p++;
}
Also, they're *conventions*, not law. The purpose of conventions is to
help us write clearer code. Don't make code less clear just to conform
to a convention. Use your judgement.
>>> 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?
>
> I just meant something like if (f1() || !f2()) { < error > }, nothing special.
Perfectly consistent error checking style, idiomatic C, pick one.
>>> 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.
>>
>
> Sounds good. But we are speaking about all functions, not only with errp
> parameter.. Or not?
The conventions I described apply to error checking, regardless of Error
object use. For instance, they'd apply to error-checking
remove("some-file").
- Re: Questionable aspects of QEMU Error's design, (continued)
Re: Questionable aspects of QEMU Error's design, Markus Armbruster, 2020/04/02
- Re: Questionable aspects of QEMU Error's design, Vladimir Sementsov-Ogievskiy, 2020/04/02
- Re: Questionable aspects of QEMU Error's design, Peter Maydell, 2020/04/02
- Re: Questionable aspects of QEMU Error's design, Daniel P . Berrangé, 2020/04/02
- Re: Questionable aspects of QEMU Error's design, Markus Armbruster, 2020/04/02
- Re: Questionable aspects of QEMU Error's design, Vladimir Sementsov-Ogievskiy, 2020/04/02
- Re: Questionable aspects of QEMU Error's design, Markus Armbruster, 2020/04/02
- Re: Questionable aspects of QEMU Error's design, Vladimir Sementsov-Ogievskiy, 2020/04/02
- Re: Questionable aspects of QEMU Error's design,
Markus Armbruster <=
Re: Questionable aspects of QEMU Error's design, Paolo Bonzini, 2020/04/02
Re: Questionable aspects of QEMU Error's design, Daniel P . Berrangé, 2020/04/02
Re: Questionable aspects of QEMU Error's design, Alex Bennée, 2020/04/02
Re: Questionable aspects of QEMU Error's design, Eric Blake, 2020/04/02
Re: Questionable aspects of QEMU Error's design, Markus Armbruster, 2020/04/04
Re: Questionable aspects of QEMU Error's design, Markus Armbruster, 2020/04/27