[SSSD] [PATCH 0/4] [REBASE-1] Introduce SSSD specific error values.

Simo Sorce simo at redhat.com
Thu Feb 28 15:36:20 UTC 2013


On Thu, 2013-02-28 at 10:29 -0500, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 01/12/2013 03:29 PM, Simo Sorce wrote:
> > Rebased on master, this patchset depends on the 2 patch krb-child
> > refactor patchset.
> > 
> > Simo.
> > 
> > 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          |   51 +++++ 
> > src/util/util_errors.h          |   85 +++++++++ 10 files changed,
> > 418 insertions(+), 334 deletions(-) create mode 100644
> > src/util/util_errors.c create mode 100644 src/util/util_errors.h
> > 
> 
> 
> I have several general concerns about this set of patches. I agree
> with the intent.
> 
> 1) Please don't use ERR_ as the prefix. I'm wary that we'd eventually
> have a conflict with some other library that we decided to import.
> Perhaps SSS_ERR_* instead?

Too long, I know of no librray that uses ERR_, I say we should worry
when we find one.

> 2) Can we please use 'enum sssd_errors ret' instead of 'int ret' in
> the code for all new additions?

No, we use errno_t.

>  It adds better compile-time checking
> (and possibly optimizations) and will make it easier to transition,
> since as we switch to using this, it will reveal any attempts we make
> to assign an errno value to it (even if it's coming from another
> function).

No because we use the full range of errors, including errno and krb5
errors, using enum would be simply wrong.

> 3) Similarly, I'd like for our new functions to have 'enum
> sssd_errors' as their return type. It will make it explicitly clear
> what they are meant to be returning.

As above, no.

> Minor nitpick on the first patch: util.h and util_errors.h both
> #require each other. If they are co-dependent, we probably want to
> handle this with 'extern' rather than a circular requirement.

Nah I am going to remove the include from util_errors.h, it should never
be included directly anyway, just via util.h

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list