Code quality news

[ghudson@MIT.EDU]

With the krb5 1.7 release out, we have the opportunity to spend a
little time on general code quality before getting back into the press
of new features and new release-critical bugs.  Here is some stuff
we've been thinking about in meetings and/or working on:

1. Stylistic consistency

We have a reasonably complete set of coding guidelines at
http://k5wiki.kerberos.org/wiki/Coding_style, but the conformance of
our existing code to those standards is not great.

Because stylistic code modifications create barriers to automatic
merges, it's wise to condense such modifications into small numbers of
events.  We have been talking about doing a "great reindent" some time
in the near future to eliminate tabs, remove trailing whitespace, and
possibly regularize some other aspects of formatting (function
definition headers come to mind).  Taking this measure shortly after a
release will make it harder to automatically backport changes to the
1.7 branch, but is less likely to get in the way of feature work
taking place afterwards.

2. Splitting up big source files

Some of the specialized applications of krb5 involve subsets of the
libraries--a well-known example is the kernel krb5 code necessary to
support the NFS client and server.  Although it's hard to structure
the source code to handle every possible subset, it's generally easier
to build subsets out of smaller pieces.

So, in the near future, perhaps just after the great reindent, we
might move code around to minimize undesirable combinations of
functions in the same library source file.  Some principles which
might apply are:

  * Code for different instances of a framework should not live in the
    same file.  (Example: we shouldn't have all of our client preauth
    mechanisms living together in preauth.c and preauth2.c.)

  * Helper functions used by several different API functions should
    not live in the same source file as one of the API functions.

  * Code used only by clients, application servers, or the KDC should
    not be mixed with code used by other categories.

3. Smaller functions

We have not made a cultural practice of using helper functions to cut
down on the size of complicated operations; as a result, we have a lot
of functions which are slightly beyond the ability of most programmers
to comprehend all at once, and a few monsters which are off the chart
(like krb5_get_cred_from_kdc_opt after referrals support).  Although
it's too much work to do it all in one fell swoop, we would ideally
like to refactor these into smaller component pieces.

For new changes, please favor the use of helper functions over
embedding too much logic into an already large function.  r22272
(ok-as-delegate stripping) is an example: the question "is this TGT
for a local realm" was farmed out to a helper function, while the core
logic of stripping the ok-as-delegate flag was kept in
krb5_get_cred_via_tkt where it could be expressed clearly with a
minimum of clauses.

4. Function documentation

We are thinking about adding doxygen markup before every function in
our libraries, starting with placeholders.  This is a departure from a
previous plan to add doxygen markup to our header files.  Non-API
functions would be marked as such using the @internal special command.
There are a few goals here:

  * Allow the generation of API documentation (suppressing internal
    function documentation) while still using the same mechanism to
    document internal and external library functions.

  * Encourage developers modifying a function to document it if it is
    not already documented.

  * As a specialized use, make it easier to automatically find
    functions within library source files.  It is not clear if this
    will turn out to be a good goal because there are other ways to do
    it (such as looking for left braces at the beginnings of lines)
    and because we may want to use Doxygen markup to document data
    structures as well as functions.

5. Test suite quality

We have a pretty big test suite, but it has some notable gaps in
coverage; examples include no testing of preauth (including PKINIT)
and no testing of the LDAP back end.  We have some unit tests written
in C with a very loose set of conventions, and some functional tests
written using dejagnu.  I am a bit dissatisfied with the dejagnu part
of the test suite, particularly in the difficulty of going from a test
failure to an analysis of the failing code path.  Recently we've added
some tests using Python (although I don't think they're integrated
into make check), and we may do more of that in the future.  We don't
have any specific plans for creating a strong framework for unit and
functional tests, but this is a recognized problem area.

We also don't have a good way of computing the code coverage of our
test suite.  In the past couple of days I've experimented with gcov,
which seems really cool but doesn't support shared libraries.  It
would be "a simple matter of programming" to do code coverage with
valgrind, but it doesn't look like the programming has been done in a
suitable form yet.  One option here is to bring back static linking
support in the build system, and modify the plugin layers to allow
static linking of plugin back ends instead of dynamic loading.  Then a
testing could be done in a fashion more compatible with
instrumentation tools.

In the long run, I would also like to make more of the krb5 code base
unit-testable, because it's often cumbersome or impossible to get good
branch coverage with functional tests.  Right now you can't unit-test
any code which talks to the network; I'm thinking of adding some
testing hooks which would allow that.

6. Static analysis

We've recently renewed our license for Coverity Prevent.  I've been
focusing on defects found in libkrb5 over the past few months, and the
number there has declined from an initial 232 to 12 (all of which are
believed to be pretty harmless).  Of course, we have other important
libraries like gssapi and crypto, but I'm still pleased with the
progress on that front.