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
While working on a different refactoring patch I came across the
krb5_child code and found it very hard to read and follow due to the
growth it has gone through in the past.
I couldn't force myself not to refactor it to make it more linear and
easier to follow and read. A bunch of dead stuff that really confused me
initially also got removed.
(I propose this for master only)
Simo Sorce * Red Hat, Inc * New York
Looks like some LDAP servers are able to store empty attribute values.
For some servers it results in syntax error, so it is probably a rare
case (if not an error) to have such values. LDB seems to be unable to
handle such values too, so in this patch we skip empty values and do not
store them to local cache.
Patch is attached.
In our responders we have a feature that allows to avoid dispatching
multiple identical requests at the same time.
This is done by queing additional identical requests in a special hash
table and then replying to all clients at the same time when a response
When the connection to a provider breaks all these waiting requests need
to be cleaned out and the has table emptied.
This was not happening in sssd_pam, so if a connection to the provider
was lost client request would pile up and never be replied again.
The attached patch fixes this issue. Tested using kill -STOP of the
sssd_be process and trying to authenticate via the gnome screensaver.
Simo Sorce * Red Hat, Inc * New York
The result of the percent calculation was always 0 as it used plain
ints. The patch switches to using explicit floats to avoid reintroducing
the bug again even with brackets.
the attached patch might seem simple, but it touches a delicate and
low-level part of the SSSD, so I'd like to request a detailed review.
I was notified by Bruce Aiesi of the Red Hat QE team that the sssd_nss
process grew its memory footprint when stress-tested by logging in many
users in parallel. I ran a couple of tests and then found out that the
responder context grew because we never freed the internal Data Provider
request. Attached is a patch that simple frees the "side request" after
all the callbacks have been invoked.
I didn't see the memory growth anymore during my testing and I didn't
see any other issues, either during my tests or with valgrind.
I hope I didn't miss any detail and the patch is OK.