On Sun, 2008-10-19 at 23:22 +0200, John Hay wrote:
> On Sun, Oct 19, 2008 at 10:51:02PM +0200, Bernd Petrovitsch wrote:
[...]
> > On Son, 2008-10-19 at 21:30 +0200, John Hay wrote:
> > [...]
> > > So here is my patch to clip the mtu in the IPv6 case. I have hardcoded
> > > OLSR_IPV6_MTU, but at least on the BSDs it is defined in netinet/ip6.h as
> > > IPV6_MMTU. It did not look as if ip6.h is pulled in at the moment, so I did
> > > not know if we should start now. :-)
> > 
> > Why not?
> 
> Well looking through interfaces.h and defs.h, it seemed to hardcode
> some of the network related values, so I was not sure which way to

Hmm, these were there before my time. Actually I never thought about
them before.

> go. Also I wasn't sure if it was a standard define in a standard
> place. I found quite a few BSD references, but not much else.
> 
> > Is there any reason not to use a #define in a system header and hardcode
> > magic values in some application?
> > 
> > Hmm, on my Fedora-9, I can find
> > ----  snip  ----
> > /* The latest drafts declared increase in minimal mtu up to 1280. */
> > 
> > #define IPV6_MIN_MTU	1280
> > ----  snip  ----
> > in <linux/ipv6.h> which seems to be the equivalent value.
> 
> Ok, so linux use a different define in a different file, OSX will
> probably be like the BSDs, so that leaves Windows... What else is
> there to worry about?

Nothing - if no one complains, it works or no one uses it;-)

I tend to judge compile time breakage as quite harmless as they should
be easy to fix.
I just did `find /usr/include -name '*.h' | xargs fgrep -w 1280` - after
IPV6_MMTU didn't show up - and the above turned up.

As a general strategy (nice word;-):
I would take already #define'd constants instead of adding the n+1
#define for them.
That adds the problem of "different names for the same thing in
different OSs". Personally I would use the more descriptive/usual/easier
to understand in the source and add a compatibility #define for the
others.
So for the current case: Use IPV6_MIN_MTU everywhere and add a
	#define IPV6_MIN_MTU IPV6_MMTU
under a appropriate #ifdef[1].

If the (olsrd-)local interpretation is some what different (e.g. that
value is the actual always used MTU and not the minimum possible):
Use a new name and have #if ... #else ... #endif

I see no problem if there is one (or more) header file which hides such
compatibility issues (and I actually wonder why people can't agree on
the same #define'd name for the same thing).

For the current case:
But reading the patch once again (and believing that I understand;-):
Perhaps renaming OLSR_DEFAULT_MTU to OLSR_IPV4_DEFAULT_MTU (or
IPV4_DEFAULT_MTU[0]) and adding OLSR_IPV6_DEFAULT_MTU is the most clear
way to go.
Or did I miss something?

	Bernd

[0]: this olsr_ prefix feels somewhat religious.
[1]: IPV6_MIN_MTU is IMHO clearer than IPV6_MMTU.
-- 
Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services




