GNOME Bugzilla – Bug 785577
clobbers errno while setting GError
Last modified: 2017-08-03 09:22:08 UTC
As noted in bug 733821 comment 10: > Noticed the commit [commit 18f8b77] in git, and saw that in many places this > code uses errno directly, like > > g_set_error(...., _("foo..."), ..., g_strerror(errno)) > > where it really needs to do the > > int errsv = errno; > bg_set_error(...., g_strerror(errsv)) > > dance instead, to preserve the actual error, not any errno that the _() call > may set.
Created attachment 356630 [details] [review] Consistently save errno immediately after the operation setting it Prevent the situation where errno is set by function A, then function B is called (which is typically _(), but could be anything else) and it overwrites errno, then errno is checked by the caller. errno is a horrific API, and we need to be careful to save its value as soon as a function call (which might set it) returns. i.e. Follow the pattern: int errsv, ret; ret = some_call_which_might_set_errno (); errsv = errno; if (ret < 0) puts (strerror (errsv)); This patch implements that pattern throughout GLib. There might be a few places in the test code which still use errno directly. They should be ported as necessary. It doesn’t modify all the call sites like this: if (some_call_which_might_set_errno () && errno == ESOMETHING) since the refactoring involved is probably more harmful than beneficial there. It does, however, refactor other call sites regardless of whether they were originally buggy.
Created attachment 356635 [details] [review] gstrfuncs: Expand documentation for errno functions Mention that it really is a good idea to save errno before doing literally anything else after calling a function which could set it. Signed-off-by: Philip Withnall <withnall@endlessm.com>
Review of attachment 356630 [details] [review]: If we're going to touch all the code, maybe add a copy of https://github.com/GNOME/libglnx/blob/master/glnx-errors.h#L129 ? It'd be a bigger patch though. Actually a good chunk of these cases are really `err()` (i.e. they exit fatally after). I've been meaning to write a `g_fatal()` or so that wraps that. But anyways, OK by me to commit as is.
Review of attachment 356635 [details] [review]: OK
(In reply to Colin Walters from comment #3) > Review of attachment 356630 [details] [review] [review]: > > If we're going to touch all the code, maybe add a copy of > https://github.com/GNOME/libglnx/blob/master/glnx-errors.h#L129 ? > > It'd be a bigger patch though. desrt was looking at writing something similar, but specialised to not just return G_IO_ERROR. I’ll push both these patches as-is and we can follow up later with something like glnx_throw_errno() once desrt’s taken a look.
Attachment 356630 [details] pushed as 5cddde1 - Consistently save errno immediately after the operation setting it Attachment 356635 [details] pushed as f2b6c11 - gstrfuncs: Expand documentation for errno functions