[Olsr-dev] mtu problem with multicast ipv6 packets

[Hannes Gredler]

ok, do we have agreement on the final fix ?
if so please send it to me :-)

On Mon, Oct 20, 2008 at 09:32:43PM +0200, John Hay wrote:
| On Mon, Oct 20, 2008 at 11:53:34AM +0200, Bernd Petrovitsch wrote:
| > 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].
| 
| That sounds ok.
| 
| > 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 think that the idea is that for things like multicast, you should
| fall back to the minimum that is guaranteed. In the IPv6 world, that
| is 1280 bytes. For normal packets, path mtu discovery will take care
| of mtu mismatches.
| 
| > 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?
| 
| Or OLSR_IPV6_MULTICAST_MTU, but DEFAULT is keeping it closer to the
| ipv4 equivalent. :-)
| 
| > 
| > 	Bernd
| > 
| > [0]: this olsr_ prefix feels somewhat religious.
| 
| That is probably just to make sure that there is no collision in the
| name space.
| 
| > [1]: IPV6_MIN_MTU is IMHO clearer than IPV6_MMTU.
| 
| I agree with this.
| 
| John
| -- 
| John Hay -- (spam-protected) / (spam-protected)
| 
| -- 
| Olsr-dev mailing list
| (spam-protected)
| http://lists.olsr.org/mailman/listinfo/olsr-dev
|