Skip to content

Cleanup error handling #2471

Description

@Blub

We have a bunch of functions throughout with inconsistent error return values, mixing -1 with and without a valid errno with returning negative errno codes. Eg. some confile.c functions can return -EINVAL as well as just -1.
We should clean these up to follow the coding style[1] document.
In other words: in our own functions, if not in conflict with a successful return value, always if the type of error makes a difference to the caller, try to return a negative error code (iow, if the error was triggered by something setting errno, simply return -errno;), otherwise actually set errno if needed. (Eg. when a non-NULL pointer is to be returned on success, return NULL after setting errno).

[1] https://github.com/lxc/lxc/blob/master/CODING_STYLE.md#return-error-codes

EDIT: I updated the above a bit as it doesn't actually make sense to force such values to be used everywhere. The thing to look out for is whether the type of error actually makes a difference to the caller. The coding style doc actually says to use them if the function can fail in a "non-binary way". In cases where the caller cannot react to the exact error the best thing to do is to report as much diagnostic info as available at the time the error happens and then just return -1.

Activity

  1. brauner commented on Jul 16, 2018

    @brauner
    Member

    @2xsec, this might be something you might be interested in doing. Probably a good idea to either do a commit for each function or for each file.

  2. added
    FeatureNew feature, not a bug
    EasyGood for new contributors
    on Jul 16, 2018
  3. 2xsec commented on Jul 16, 2018

    @2xsec
    Contributor

    @brauner, Of course. I'll check it.

  4. brauner commented on Jul 16, 2018

    @brauner
    Member

    @2xsec, no pressure just in case you're interested. You might also consider jumping into our irc channel #lxc-dev on Freenode. But of course, that's completely optional too. :)

  5. 2xsec commented on Jul 16, 2018

    @2xsec
    Contributor

    @brauner This is no pressure for me.:) Please recommend a good tool for irc.:)

  6. brauner commented on Jul 16, 2018

    @brauner
    Member

    Hm, we usually either use irssi or weechat. But there are graphical or browser based irc clients as well. It depends on whether you want to be connected constantly or only on demand. Most of us have a persistent irc session running in tmux on a remote server so that we can simply attach to it every day and see what we missed and also have logs of all interactions.
    Personally, I use ZNC as irc bouncer but that is probably not needed.

  7. added a commit that references this issue on Jul 21, 2018
  8. Blub commented on Aug 20, 2018

    @Blub
    MemberAuthor

    Updated my intial comment a bit in response to #2551. I think this part might benefit from a few examples in the coding style doc, or some more situation distinctions.

  9. brauner commented on Aug 20, 2018

    @brauner
    Member

    @Blub, cool. From what I gathered from our recent irc discussion you'd prefer to set errno instead of returning -<errno-value> from functions, right. I'll let you review branches such as #2551 since you have a better grasp of what you're aiming for. :)

  10. Blub commented on Aug 20, 2018

    @Blub
    MemberAuthor

    I don't remember saying that ;-). Setting errno was for functions where the return type conflicts with the error codes (eg pointers.). Weren't you entertaining the idea of using PTR_ERR()? That one I'm not so fond of ;-).

  11. brauner commented on Aug 20, 2018

    @brauner
    Member

    No, PTR_ERR() and friends should stay restricted to our kernel work. ;) We agreed on that on irc a while back. errno is what we have for the userspace part of our work.

  12. 2xsec commented on Aug 20, 2018

    @2xsec
    Contributor

    @brauner @Blub I'm sorry. I didn't finish this issue because there are so many functions return int or errno values. So, I think we need to define well-defined return type. If you have any good idea, I'll try to work for this. Thanks. :)

  13. brauner commented on Aug 20, 2018

    @brauner
    Member

    I'll let @Blub spearhead this work and its direction. I'm just going to give snide comments from time to time. ;)

  14. 2xsec commented on Aug 20, 2018

    @2xsec
    Contributor

    @brauner Thanks. :)

  15. tcharding commented on Aug 20, 2018

    @tcharding
    Contributor

    I'll close #2551 then and let you boys at it.

  16. stgraber commented on Mar 20, 2020

    @stgraber
    Member

    @brauner do we still need this issue or did you accidentally clean that all up by now?

  17. changed the title [-]error handling cleanups[/-] [+]Cleanup error handling[/+] on Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    EasyGood for new contributorsFeatureNew feature, not a bug

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions