On Wed, 2008-09-17 at 00:46 -0600, Shawn M Emery wrote:
> Mark Phalan wrote:
> > I've just uploaded a webrev of my resync/pkinit workspace. There still
> > needs to be some work on pkinit so don't expect the code in
> > usr/src/lib/krb5/plugins/preauth/pkinit/ to be complete (you can ignore
> > it for now). I'll post another incremental webrev with any changes I
> > make to the pkinit code later on. The rest of the changes are resync
> > changes for MIT 1.6.3. The hg comment needs to be updated, I'll do that
> > once we get the pkinit PSARC case submitted.
> >
> > I've chunked the review up into four pieces as I expect the krb team to
> > do the review.
> >
> > Shawn: Chunk 1
> >
>
> In general looks good, thanks for further notating preexisting Solaris
> specific code.
> Per usual, with these types of changes, I'm ignoring cstyle and lint
> type comments.
>
> usr/src/cmd/krb5/kadmin/Makefile:
> 30: Why is kclient being excluded?
It shouldn't have been - when I was playing around with the Makefiles
for the new pkinit stuff I accidentally left this change in.
Fixed.
>
> usr/src/cmd/krb5/kadmin/server/kadm_rpc_svc.c:
> 38: included twice
Fixed.
>
> usr/src/cmd/krb5/kadmin/server/ovsec_kadmd.c:
> 553: is kadm5_init2 unique to Solaris? It is and should have "Solaris
> Kerberos"
Yes. Fixed.
> 600: should be consistent with log message, no whoami then
krb5_klog_syslog() will prepend a "whoami" to the message.
> 641: s/cannot/Cannot/
>
Fixed.
> usr/src/cmd/krb5/kinit/kinit.c:
> 345: k_opts not used for anything
Not sure I understand. The "struct k_opts *opts" parameter is used in
the add_preauth_opt() func.
>
> usr/src/cmd/krb5/krb5kdc/kdc_preauth.c:
> 79: why is /krb5/plugins/preauth not being used?
It should be but that fix came in with the other pkinit changes that I
haven't sent for review yet. Anyway - it is fixed in the final bits.
> 1042: should be returning ENOMEM
Agreed. Fixed.
> 1560: minus the comments, this is identical to MIT's
Similar issue for comment on line 1476.
Both removed.
>
> usr/src/lib/gss_mechs/mech_krb5/crypto/des/afsstring2key.c:
> 389: this line deviates from MIT's probably because of a lint fix,
> should revert (w/o NULL)?
I think lint will complain but I'll play with it.
> 441: this was changed because of "PSARC/2004/619 Namespace for Solaris
> on amd64",
> are the consequences know for reverting back?
The "register" keyword is simply a way to advise the compiler that the
variable will be heavily used (so it might be good to keep it in a
register). It would seem to be suitable in this situation and its what
MIT uses.
I don't have any performance comparisons of the compiled code. I don't
mind reverting if anyone has a strong opinion.
>
> usr/src/lib/gss_mechs/mech_krb5/crypto/dk/stringtokey.c:
> 71&73,102-104: just remove the voids to make this in sync w/MIT
Fixed.
>
> usr/src/lib/gss_mechs/mech_krb5/crypto/hash_provider/hash_md5.c:
> 45: comments should start before the function, as the arguments are
> unique to Solaris. You might want to be consistent and put the Solaris
> comment before the other crypto function defs.
Fixed.
>
> usr/src/lib/gss_mechs/mech_krb5/crypto/old_api_glue.c:
> the Solaris specific casting in this module was for successful compilation?
Yes.
> 341: should be returning ret here
Yup. Fixed.
> usr/src/lib/gss_mechs/mech_krb5/include/socket-utils.h:
> 77,82,87,91,96: why aren't these designated as inline?
gcc complains about "inline" when compiled on Solaris (perhaps due to
the opts we pass it?). That code however isn't used to genereate the
final library (as its gcc only) so it doesn't really make much
difference.
I've reverted anyway to the previous syntax.
>
> usr/src/lib/gss_mechs/mech_krb5/krb5/asn.1/asn1_k_encode.c:
> 765: should go before line 767
> 770: should go before line 772
I didn't put it there as it would have further separated the if()
statement from the code below it.
I agree that the comments are really in the wrong place though.. I've
updated but also added '{'/'}'s.
>
> usr/src/lib/gss_mechs/mech_krb5/krb5/ccache/cc_memory.c:
> 673: more memory leaks, should also be freeing new_node's creds after
> this as well
Nice catch.
Fixed.
>
> usr/src/lib/gss_mechs/mech_krb5/krb5/ccache/cc_retr.c:
> 255,270: just remove the void, to remove the above Solaris comment
Fixed.
>
> usr/src/lib/gss_mechs/mech_krb5/krb5/keytab/ktbase.c:
> 48,53,58: why do these differ from MIT's, shouldn't "const static" suffice?
> 77: possibly related to above
I'll change them and see if lint complains.
> 214,231: is the goal to keep these checks lint error free? Not for sure
> if these are egregious enough.
I believe the checks are for lint cleanliness.
>
> usr/src/lib/gss_mechs/mech_krb5/krb5/keytab/ktfns.c:
> 60: ah, this fixes the null realm keytab get entry error that we've seen
> with the new referral logic
Yup. I'll make sure to test this scenario and verify that it works.
>
> The rest looks good.
>
Thanks for the review.
-M
