Code quality news
raeburn at MIT.EDU
Fri Jun 5 02:19:05 EDT 2009
On Jun 4, 2009, at 13:35, ghudson at MIT.EDU wrote:
> 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:
Overall this looks very good!
> 2. Splitting up big source files
I think this is good to a point. I don't really subscribe to the "one
function per file" methodology that was so important in the days of
static-only linking, expensive disks, and every admin having to
compile and install Kerberos himself if he wanted the three or four
existing Kerberos applications; now shared libraries are the norm, the
linker can't selectively discard functions not in use by one
particular application, and unless the functionalities are really
distinct it's often helpful to see multiple functions together instead
of in separate files. As long as you're not taking it to an extreme,
yes, splitting up source files with not-terribly-closely-related
functionality is probably a good thing. If it is closely related, I'd
keep them together.
I also think it can be reasonable to use preprocessor directives for
some subsetting. Not in all cases, but maybe if you want a subset of
very closely related functions. (E.g., for cases like ccaches or
keytabs where we use function-pointer tables, I'd probably keep the
individual implementations collected together in one file each, and
use #ifdef to eliminate certain operations not needed in a certain
subset. That is, unless you want to factor out some common code or
algorithms used in multiple of these implementations as separate
I think I'd like to see k5-int.h broken up a lot. Having one or two
headers where you can get all the widely-used internal stuff is
probably good, but it can include sub-headers with different types of
functionality, and it can omit things that really could be localized
to one directory or so. The installed krb5.h, eh, maybe... it might
encourage some developers to include only <krb5/this.h> and <krb5/
that.h> unless you take steps to make doing so difficult, and then you
can never get rid of those headers if you want to reorganize again.
The glibc folks have done it well, I think -- if you try to include
the wrong header directly, certain cpp symbols don't get defined and
it yells at you and your code doesn't compile.
> 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.
True, though a "piece" could be a function instead of a file.
> * Helper functions used by several different API functions should
> not live in the same source file as one of the API functions.
If the API functions have similar functionality, I think it makes
sense to put them (and the support functions) all together in one
file. Unless you're going for the smallest-static-link goal.
Subsetting by selecting certain files makes this trickier when you
only want some of these API functions; but I think all of the
solutions are kind of ugly in that case. (I'm picturing something
like get_creds here -- big "helper" functions with all the
functionality, maybe broken into manageable pieces, wrapped with
little API functions presenting nicer interfaces for the common use
cases. If the helper functions are common to various unrelated API
functions, or the API functions involve a lot of non-common code,
yeah, make them separate instead of treating them like they're closely
tied to one use and not another.)
> 3. Smaller functions
I don't mind big functions for one big integrated piece of
functionality with non-trivial logic, but that's usually not what
we've got. Breaking the bigger functions up into logically separate
helpers would improve things a lot!
> 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.
I'm a little concerned about this. Does doxygen let you specify "this
part of the documentation for this function is internal"? I could
imagine some of the internal documentation including notes about
details of the current implementation, and wanting to make such notes
about functions that are exported, but not wanting to make them part
of the API contract that application programmers will expect us to
adhere to going forward. Such notes shouldn't show up in the official
Ken Raeburn / raeburn at mit.edu / no longer at MIT Kerberos Consortium
More information about the krbdev