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