qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Questionable aspects of QEMU Error's design


From: Vladimir Sementsov-Ogievskiy
Subject: Re: Questionable aspects of QEMU Error's design
Date: Thu, 2 Apr 2020 17:35:59 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1

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>
  }

---

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.

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

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

--
Best regards,
Vladimir



reply via email to

[Prev in Thread] Current Thread [Next in Thread]