[Ns-developers] icmpv4 stack for merging

[Mathieu Lacage]

On Sat, 2008-10-18 at 20:21 -0700, Tom Henderson wrote:
> Mathieu Lacage wrote:
> > hi,
> > 
> > Since
> > http://mailman.isi.edu/pipermail/ns-developers/2008-September/004751.html
> > 
> > I spent a while debugging this icmp stack and running it through quite a
> > lot of testing. I made a couple of changes here and there, mostly to
> > avoid generating icmp messages for multicast packets to avoid icmp
> > storms. So far, the code has been reasonably stable so, this is a
> > request for merging to craig and raj.
> > 
> > Two patches attached:
> >   - icmp.patch contains all the changes for the src/node and
> > src/internet-stack directories. 
> >   - icmp-application.patch contains the ping application and the example
> > code.
> > 
> > It would be helpful to review the first patch again. The second patch is
> > both less risky and less intrusive.
> 
> Mathieu,
> I read through your patches.  A few comments:
> 
> - I would suggest to rename SocketMtuDiscoverTag to 
> SocketSetDontFragmentTag or something like that, so it is more explicit 
> what it is functionally doing.

ok.

> 
> - likewise, in the patch, setting the UdpSocket MtuDiscover attribute 
> currently just sets DF in the outbound direction but doesn't actually 
> perform path MTU discovery AFAICS (it doesn't look like anything is done 
> with a received DestUnreach)

It is forwarded to the relevant socket, but, yes, there is no path mtu
discovery because there is no room in the forwarding table to store the
mtu.

> 
> I think the behavior you want to implement, if MtuDiscover attribute is 
> set, is the following:
> i) set the DF bit
> ii) keep a member mtu variable around that is initialized to the 
> outgoing interface MTU

The problem with the above is that we have to do a route lookup for
every outgoing packet: we can't do a single lookup at the start and,
then, assume that the result of the lookup will be the same all the time
because:
  1) the destination address could change
  2) the forwarding table content could change

> iii) update this local mtu variable upon receipt of ICMP messages
> iv) if (p->GetSize > mtu) then set EMSGSIZE errno

I was really hoping that, instead, the mtu would be set in the
forwarding table when/if you complete your ipv4 rework. 

> Otherwise, I think it looks good.  Although the ipv4-interface changes 
> will have to be updated with the forthcoming ipv4 changes, I don't think 
> anything needs to wait on the merge for ns-3.3.

Mathieu