Re: Questionable aspects of QEMU Error's design

[Vladimir Sementsov-Ogievskiy]

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

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

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.

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

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

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

--
Best regards,
Vladimir