For a long time I felt constrained using just the error values that
are available in system headers, they do not express the full range
of errors we need to express within SSSD.
So this is a RFC patchset that demonstrates how we could about getting
a richer set of error messages for SSSD that should also simplify some
of our most convoluted code when it comes to returning errors.
The first patch just introduces a very simple error number space and 2
basic errors we always missed. ERR_INTERNAL is what we wanted to use in
many places where we currently return EIO or EFAULT for example.
Patch 2 shows how we can dramatically improve error reporting for our
offline caching authentication code with just a few more targeted errors.
The main thing to note here is that now we can safely distinguish from
a real system error about a missing file (ENOENT) and actually missing
Patch 3 is just an example of fixing EIO in a commonly used macro.
Patch 4 is the most complex example of converting a whole subsystem
that is really complex in its error handling.
Even after the recent refactoring patchesi, krb5_child error reporting
remained quite complex as it uses all of errno, krb5 and pam status
The result is that we have to track multiple different error variables
and that can easily lead to hard to read code as well as bugs where one
error code does not reflect into others.
With this patch I think we show how we are able to reduce the complexity
of error handling and at the same time return precise information on what
NOTE: This patchset depends on my previous patches for refactoring the
krb5_child as well as the patches that kix the krb5_auth style and the
patches that introduce the authtoken structure.
Simo Sorce (4):
Add SSSD specific error codes and definitions
Use SSSD specific errors for offline auth
Return ERR_INTERNAL instead of EIO
Cleanup error message handling for krb5 child
Makefile.am | 4 +-
src/db/sysdb_ops.c | 17 +-
src/providers/krb5/krb5_auth.c | 157 +++++++++-------
src/providers/krb5/krb5_child.c | 388 ++++++++++++++++-----------------------
src/tests/auth-tests.c | 6 +-
src/tests/sysdb-tests.c | 12 +-
src/util/auth_utils.h | 22 ++-
src/util/util.h | 10 +-
src/util/util_errors.c | 50 +++++
src/util/util_errors.h | 85 +++++++++
10 files changed, 417 insertions(+), 334 deletions(-)
create mode 100644 src/util/util_errors.c
create mode 100644 src/util/util_errors.h
I noticed the warning while testing another patch.
Associate Software Engineer
Red Hat Czech
loc: 1013 Brno 1 office
irc: okos @ #sssd #brno
In most cases the bug would have no effect because we just use becli for
mem_ctx as well, but in some cases, like subdomains, NULL is passed for
the becli, so we might leak the request if we weren't careful.
I study at FIT VUT in Brno and as a part of my bachelor thesis I'm
creating a GDB extension for tevent library. I would like to ask you
for your suggestions - what functions of such a plug-in would you use?
What would you consider useful? Number of requests in a queue? List of
events in progress? ...
So far I've implemented commands giving numbers of allocated events
(divided according their types); showing state of specified request;
watching for changes of requests' states.
I would appreciate any comments.
There was another bug in pam_pwd_expiration warning. If it was set to 0
which means "always display whatever server sends", the condition
wouldn't hit and effectivelly the option would be set to -1 (=use
since the SSSD development started, the codebase used only error codes
from the standard errno range (see man errno.h). Recently it has became
apparent that it's not really sufficient -- the errno codes are too tied
to glibc interface and system calls and do not really reflect what is
going on in the SSSD. Quite often, we would misuse "EIO" or "EFAULT" for
"something is wrong".
There are cases where we want to accuratelly report the particular error
code to an external interface (such as pam_sss). Previously we would just
special case some errno codes, but that turned out to be too buggy and
hard to follow. See upstream bugs like #1310 or #1699 for an example.
To solve this issue, Simo proposed introducing an internal range or SSSD
specific error codes:
I think his patches are very nice in general. I reviewed the patches in
person this weekend on http://devconf.cz and when I run some more tests
I'd like to push them.
To sum up our discussion for the other developers, here are some guidelies
to follow. In general, after the patches are pushed, the new code should
use the SSSD specific error codes, not errno internally.
* If the current error codes do not fit, the developer should
introduce new ones. The new error codes should be specifically
called out in the commit message or even split into a separate
patch as appropriate. In general, the error codes should converge
over time to a stable set that wouldn't grow any more.
* When introducing new error codes, the developer should make sure
that consumers of the interface being introduced or converted
does not special case some old errno-codes (we often
special-case EAGAIN to denote "request still in progress" for
* the error codes would not be split into any ranges. If the
caller needs to check N error codes, it should check N separate
error codes (or refactored)
Please raise any concerns you might have over the plan.