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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
