On Fri, 2013-03-08 at 12:18 -0500, Nathaniel McCallum wrote:
> On Fri, 2013-03-08 at 13:09 +0100, Jakub Hrozek wrote:
> > On Tue, Mar 05, 2013 at 03:58:54PM -0500, Nathaniel McCallum wrote:
> > > This email provides the patch as an attachment. It also provides a more
> > > substantial commit message and fixes one comment formatting issue.
> > >
> > > Nathaniel
> >
> > Hi Nathaniel,
> >
> > thanks for the patch! I'm not familiar with the OTP development in
Kerberos
> > much, so I would prefer if there was additional opinion on this patch.
> >
> > Functionality-wise, I only tested that with with krb5 1.10 and 1.11, when
> > using passwords, nothing breaks and I could still log in.
> >
> > Code-wise, do you expect that the code in krb5_child.c will grow much when
> > implementing the missing features? If so, you might want to split it into
> > a new file. krb5_child.c is getting quite big already.
> >
> > We have recently introduced a new set of error codes to the SSSD that
> > would gradually supersede the errno codes (for anything other than libc
> > API that really uses errno). It would be nice if the patch introduced new
> > error codes to src/util/util_errors.h instead of using EPROTO or ENOTSUP. I
> > know your development predates these new error codes, but I'd still prefer
> > if new code would use these error codes instead of errno.
> >
> > Couple of other comments inline.
>
>
https://github.com/npmccallum/sssd/commits/master
>
> All the inline issues are fixed.
>
> Since all the errors produced by tokeninfo_matches() are consumed
> immediately and tokeninfo_matches() can never be used outside this
> context, I am not inclined to create new global error codes for this
> local usage.
Patch attached.
All my concerns except for the error codes which we decided not to
change at this point and one condition that is not enclosed in {}
brackets (I'll fix myself before pushing no need to resend) have been
addressed.
Ack from me.