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