Difference between revisions of "Cleanups"

Revision as of 23:53, 26 June 2012

This page describes opportunities for internal code cleanups. Anything to do with a user-facing or developer-facing behavior should go elsewhere (perhaps in an RT ticket, or somewhere related to the roadmap).

• We have two different implementations of reading the same realm params, one used by the KDC (krb5_read_realm_params and adm.h) and the other used by everything else (kadm5_get_config_params and kadmin/admin.h). The KDC should be switched over to kadm5_get_config_params and krb5_read_realm_params and its header should be removed.

• sam2_process (preauth_sam2.c) should use a cleanup label instead of repeated free invocations. It is also long and should be broken up into smaller pieces if possible. (See Manual Testing for guidance on how to test at least part of this function.)

• kadmind's main() should not have repeated blocks of calls to release resources. It's probably unnecessary to release resources on error exit (plenty of other programs don't bother).

• k5-utf.8 uses its own conditionals for fixed-length types instead of the ones from krb5.h. On a related note, ucdata.h uses krb5_ui_4 instead of krb5_ucs4 but callers in ucstr.c use krb5_ucs4 pointers, generating warnings.

• The build system used to have multiple configure.in files making use of an aclocal.m4. Now that we only have one configure.in, we don't really need a separate aclocal.m4.

• Not all variables in the build system's Makefile fragments (like pre.in) are used. For instance, SHLIB_TAIL_COMP and KRB5_SHLIBDIR appear to be unused. Some variables like INSTALL_PREFIX are just name variants of other variables and are only used in one or two places under the variant name.

• Account lockout decision logic is duplicated in the DB2 and LDAP KDB modules.

• The serialization framework has several levels of unnecessary nesting. Also, it passes every serialization call through a table lookup of magic numbers, which is unnecessary since we always know what we're serializing (except for krb5_serialize_data, which is not used outside of test programs and is not a public API despite the name).

• lib/krb5/os appears to have a couple of different functions to generate ADDRTYPE_ADDRPORT addresses (one in mkfaddr.c and another in full_ipaddr.c).

• klist and kinit (at least) still have some internal vestiges from when they supported krb4 and krb5 credentials, and could be simplified.

• The kdcpreauth and clpreauth pluggable interfaces use the new plugin support functions, but do not have proper consumer interfaces as described in Projects/Plugin_support_improvements#Consumer_interface.

• kdc_preauth.c could be simplified by simply inserting informational padata values into the returned sequence, rather than having fake preauth systems for each informational preauth type.

• It's no longer our practice to use krb5_xfree() instead of free(), but it's still used in a few places (kdb5.c and the LDAP KDB module).

• It's no longer our practice to use krb5_x() to make calls through function pointers, but it's still used in a few places (ktfns.c and rcfns.c).

• Some kdc_util.c functions take krb5_db_entry structures instead of pointers.

• The LDAP KDB module maintains a connection pool, but only ever uses one connection (since our KDC is single-threaded).

• krb5_rc_default and krb5_rc_default_name are not part of our public API and are not used in the tree.

• Various parts of lib/gssapi still use K&R-style function definitions.

• lib/gssapi/mechglue should use the MIT krb5 style, not the Solaris coding style with tabs.

• lib/gssapi/generic contains some utility functions which are specific to the krb5 mech and should be located in lib/gssapi/krb5.

• Some parameters in osconf.hin are unused (like KDC_PORTNAME and KDC_SECONDARY_PORTNAME), or appear to be used but are never really used (like KRB5_DEFAULT_ADMIN_ACL and DEFAULT_ADMIN_ACL).