[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