[OpenAFS-devel] Refactoring the Solaris libafs code base

chas williams - CONTRACTOR chas@cmf.nrl.navy.mil
Wed, 03 Jan 2007 10:43:17 -0500


In message <200701022320.SAA18933@quince.ifs.umich.edu>,Marcus Watts writes:
>OpenAFS has its own local set of conventions which have evolved but
>don't seem to be well documented or consistently followed.  There's
>some stuff in README.DEVEL (from jhutz@? ) but it's missing anything on
>include files.  It's nearly always a good idea to include "afsconfig.h"

it would probably be a good idea to expand on this document a bit.
like when to use afs_osi_XXX instead of osi_XXXX.  some other suggestions
i would like to see might be how to handle the ifdef clutter.

i would suggest adopting the idea from the linux kernel.  use the 
new interface in the code, and stickl the ifdef clutter in a header.
this will make the relevant code easier to read:

instead of:

	#ifdef CONFIG_PM
		if (
	#ifdef PF_FREEZE
		    current->flags & PF_FREEZE
	#else
		    !current->todo
	#endif
		    )
	#ifdef LINUX_REFRIGERATOR_TAKES_PF_FREEZE
		    refrigerator(PF_FREEZE);
	#else
		    refrigerator();
	#endif
	#endif
	#endif

you would write

	try_refrigerator(current);

and somewhere else have a static inline in osi_machdep.h that 
handles the #ifdef brain damage foisted upon us.

in the case when someone makes an operating system local version of
a function, like afs_osi_suser(), dont use #ifdef OPERATING_SYSTEM
to remove the common version.  define a symbol like HAVE_AFS_OSI_SUSER
in your osi_machdep.h and use this to remove the common version.
later if others make a local version, we wont wind up with #ifdef's
that look like #if !defined() || !defined() || !defined().

also, would anyone object to moving some the operating system specific
bits of various common routines to child function.  afs_NewVCache()
is a good example.  a bit of this is specific to each platform
and could be move to something like afs_osi_NewVCache() instead of
the cut and paste mess with #ifdef's.  there are some os specific
bits that will need to remain, but it could be a bit clearer.