[OpenAFS-devel] Refactoring the Solaris libafs code base

[Marcus Watts]

"chas williams - CONTRACTOR" <chas@cmf.nrl.navy.mil> writes:
> Message-Id: <200701031543.l03FhHOn026310@cmf.nrl.navy.mil>
> To: Marcus Watts <mdw@umich.edu>
> Cc: openafs-devel@openafs.org
> In-reply-to: <200701022320.SAA18933@quince.ifs.umich.edu> 
> From: "chas williams - CONTRACTOR" <chas@cmf.nrl.navy.mil>
> Subject: Re: [OpenAFS-devel] Refactoring the Solaris libafs code base 
> Date: 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.
...

Uhhhh....

Ok this is long, but maybe I better say all this.

The problem with using the linux kernel as a coding example
is that it's extremely specialized to its one tool chain.
It's never going to build with anything but gcc/gmake.
The solaris kernel has (had?) a smaller number of equally
interesting different dependencies on its tool chain, as does
plenty of microsoft code.

Yes, I know inline functions can do nifty things, but I also think of
this as being in part a pernicious influence from c++.  The problem
with inline functions is that it's easy to abuse them to get bad code
bloat, and debugging anything real complicated with them can get very
very weird.  What does a breakpoint mean in an inline function?

Also, I would recommend being *sure* inline functions perform
equivalently between all supported systems before relying on it.  ibm,
sun, microsoft, & gnu do not implement quite the same language.  I'm
sure this hasn't been a safe feature to use in the past, but this may
not be true anymore.  As I recall, it was important to make sure the
word "static" appeared, else some compilers would insert a
gratuitous "external static" copy in as well in case you called it out
of line from another object module.

	{ I got caught by another "gcc" feature.  In
	gcc you can say things like
		typedef struct _d { unsigned char *d; int length; } d;
	and then
		static d foo = { d: ""};
	or almost.  Firstly, you get a signed char warning from newer
	versions of gcc.  Secondly, this won't work at all for sun
	workshop c.  Oh well. }

In general, if you don't need loops, local variables, or any
other unique feature -- a simple #define is I believe preferable.
Yes, it's a useful trick to define the old interface in terms of the
new, assuming as is usually the case that the old case has subset
semantics of the new interface (and that you don't need semantics
outside of that subset).  There are also things you can do with
a #define that you can't do with a C inline.  (Or at least couldn't;
in c++ you can and it could be spreading...)  For instance,
treating parameters as lvalues.

If you do use '#define', then there are some things to keep
in mind.  For instance, it's dangerous to have more than one statment
in one #define, because it won't behave right with "if".  
Bracket with MACRO_BEGIN MACRO_END if you can to do this.
Sometimes, statments can be separated with , instead.
This is because many statements are expressions, and
e,e is valid C.  But in this case it's better to say (e,e) instead
so that it works right as a parameter in a subroutine call.
See "rx/rx_multi.h" for a particularly specialized case of ; in #define.

With that in mind, yes, absolutely, I'm all for using simple clever
tricks to reduce the number of ifdefs's.  Moving things into OS specific
files is not a bad approach- in moderation.

			-Marcus Watts