On Tue, 2008-03-25 at 17:18 -0600, Shawn M Emery wrote:
> This is a request for a code review of the following changes:
>
> PSARC/2007/401 kclient version 2
> 6287615 kclient enhancement to support domain joining for AD interop
> 6263626 kclient does not accept 'search' type lines in resolv.conf
> 6362266 kclient doesn't support aliasing KDCs
> 6405691 kclient should be used to configure DHCP/VPN clients and for
> non-Solaris KDCs
> 6629530 kpasswd(1) in SET_CHANGE mode should try kpasswd_server first
>
> Webrev can be found here:
> http://cr.opensolaris.org/~semery/6287615
Here's what I've got:
kadm_host_srv_names.c: 112
Return value not checked - could end up passing garbage to
"profile_get_string"
kadm_host_srv_names.c: 125
Return value not checked.
kadm_host_srv_names.c: 133
Return value not checked.
client_init.c: 312
'hp' should be initialized to NULL otherwise the code path at
line 352 may not be taken
exception_list_i386: 573
Why krb5kdc?
exception_list_sparc: 578
Why krb5kdc?
klookup.c: 41
Comment should be improved at least to mention DNS.
ksetpw.c: 79,93,99,108,131,153,159,169,177,194
gettext() should be used for error message string.
krb5_get_error_message() should be used to print the string
reprsentation of the Kerberos error (where applicable).
com_err() could probably be used.
ksetpw.c: 122
Return value of malloc() not checked.
ksetpw.c: 124
Return value of realloc() not checked.
ksetpw.c: 169
The error message could include "vprincstr"
ksetpw.c: 173
Use "stdin" instead of '0'?
ksetpw.c: 174
Why 300? Instead of '300' a constant may read better
ksetpw.c: 181
Return value of strdup() not checked.
ksetpw.c: 183
gettext() should be used
ksetpw.c: 187
Return value of strdup() not checked.
ksetpw.c: 207:
Print error message for kt_remove_entries() failure.
ksetpw.c: 341-346
gettext() should be used.
kdyndns.c: 41,42
gettext() should be used.
kdyndns.c: 81
Return value of smb_nic_init() not checked
kdyndns.c: 83
Return value of smb_nic_fini() not checked.
ksmb.c: 43-47,98,105
gettext() should be used.
ksmb.c: 95
Use "stdin" instead of '0'?
ksmb.c: 96
Why 300? Instead of '300' a constant may read better
ksmb.c: 102,108
Return value of strdup() not checked.
kclient.sh:
46,47,57,193,821,1009,1010,1023,1044,1105,1114,1174,1240,1311,1312
Message should to to stderr
kclient.sh: 76,78
TPAM initialized twice
kclient.sh: 78
mktemp may fail
kclient.sh: 137, 145, 170
kadmin is using $KRB5CCNAME but the "kinit" command never
populates the cred cache (kclient.sh:583,585)
kclient.sh: 139,141,147,150,174
How will this work in a non-C locale?
kclient.sh: 290
Why no_addresses = true?
kclient.sh: 308-327
Looks wrong when English isn't being used
kclient.sh: 85
Missing '$' before 'KLOOKUP'
kclient.sh: 567-577
What if the admin principal isn't using the name/admin convention?
kclient.sh: 583, 585
Output of kinit should be suppressed/captured
Return code should be checked.
kclient.sh: 588
Looks wrong when English isn't being used
kclient.sh: 645
gettext should be used for "System"
kclient.sh: 936
what if the interface has an address and a netmask but isn't UP?
what about inet6?
kclient.sh: 1257, 1303
cryptoadm would seem to be a more natural interface than "encrypt"
when determining supported crypto mechanisms.
kclient.sh: 1333,1354
Return code not checked.
kclient.sh: 1377, 1764
Any reason '-rf' wasn't used?
kclient.sh: 1439-1451
uid checks against 0 aren't kosher any more. The user may have
the relevent execution profiles/authorizations/privileges. The
error messages should inform the user what execution
profiles/auths/privs he needs to run the script.
kclient.sh: 1700
'svcadm enable' should be run with '-s' and have its return code
checked.
-Mark
