Novell and MIT moving forward on LDAP Plugin
Luke Howard
lukeh at padl.com
Thu Jun 29 22:09:17 EDT 2006
A couple of quick comments on the code itself:
1) Is it really necessary to use goto when there is nothing to cleanup?
eg. in krb5_ldap_read_server_params() or krb5_get_int_from_tl_data()
2) Code formatting should be internally consistent and consistent with
the rest of the Kerberos source, in terms of bracing, spacing in variable
declarations and branches, etc.
eg.
static krb5_error_code
krb5_get_int_from_tl_data(context, entries, type, intval)
krb5_context context;
krb5_db_entry *entries;
int type;
int *intval;
{
krb5_error_code st=0;
krb5_tl_data tl_data;
void *voidptr=NULL;
int *intptr=NULL;
tl_data.tl_data_type = KDB_TL_USER_INFO;
if (((st=krb5_dbe_lookup_tl_data(context, entries, &tl_data)) != 0) || tl_data.tl_data_length == 0)
goto cleanup;
if (decode_tl_data(&tl_data, type, &voidptr) == 0) {
intptr = (int *) voidptr;
*intval = *intptr;
free(intptr);
}
cleanup:
return st;
}
The use of goto disguises the fact that the function will return 0 even
when tl_data.tl_data_length is 0 -- is this desired? Also the pointer
casting could be considerably simplified.
3) Rather than the LDAP_SEARCH macros I would use functions, it makes
debugging easier.
4) Avoid C++ comments.
5) #includes should be at the top of the file, eg. in kdb_ldap.c.
6) All files should have copyright notices, all headers should have
guards.
These comments are based on a quick brows through the ldap-integ
branch, I will endeavour to provide more detailed comments.
-- Luke
--
More information about the krbdev
mailing list