OK, now with more time to write this up, here's the dilemma.

In the Ipv4L3Protocol tx/rx methods we are able to report all the following combinations of information to the trace sources, all with equal cost:

1. Ipv4Header&, Packet-without-ip-header
2. Ipv4Header&, Packet-with-ip-header
3. Packet-with-ip-header

And we have the following use cases:
1. Ascii/pcap tracing
2. Flow Monitor

For the tracing use case, a packet-with-ip-header is preferable.  If the packet-without-ip-header is passed then we need to first Copy() the packet, AddHeader(ipHeader), and only then can we write it to the trace file.

For the Flow Monitor case, due to the need to look into the L4 header, packet-without-ip-header is preferable. If the packet-with-ip-header is passed then we need to first Copy() the packet, RemoveHeader(ipHeader), and only then can we peek into TCP or UDP header, using PeekHeader().  OTOH, if If the packet-without-ip-header is passed then we can skip the Copy() and RemoveHeader() expensive operations and go directly to the PeekHeader(tcp_or_udp) stage.

So there are two antagonistic use cases here, with different requirements from the trace sources.  Whichever we choose, we lose something.  Any ideas?

(In reply to comment #1)
> OK, now with more time to write this up, here's the dilemma.
> 
> In the Ipv4L3Protocol tx/rx methods we are able to report all the following
> combinations of information to the trace sources, all with equal cost:
> 
> 1. Ipv4Header&, Packet-without-ip-header
> 2. Ipv4Header&, Packet-with-ip-header
> 3. Packet-with-ip-header
> 
> And we have the following use cases:
> 1. Ascii/pcap tracing
> 2. Flow Monitor
> 
> For the tracing use case, a packet-with-ip-header is preferable.  If the
> packet-without-ip-header is passed then we need to first Copy() the packet,
> AddHeader(ipHeader), and only then can we write it to the trace file.
> 
> For the Flow Monitor case, due to the need to look into the L4 header,
> packet-without-ip-header is preferable. If the packet-with-ip-header is passed
> then we need to first Copy() the packet, RemoveHeader(ipHeader), and only then
> can we peek into TCP or UDP header, using PeekHeader().  OTOH, if If the
> packet-without-ip-header is passed then we can skip the Copy() and
> RemoveHeader() expensive operations and go directly to the
> PeekHeader(tcp_or_udp) stage.

My proposal to close this out is to use two trace signatures in this regard:

1. Ipv4Header&, Packet-without-ip-header
  - new Flow monitor traces, plus the existing drop trace
2. Packet-with-ip-header
  - existing txTrace and rxTrace

So, effectively, this doesn't require changes to the traces except for some documentation improvements.

The existing design is such that when the txTrace and rxTrace is hit, this is effectively at the lower boundary of this model (close to the NetDevice) so conceptually the IP header is on the packet.  However, within the Ip layer, the packet travels around without an IP header, so it seems like it would be cleaner to just give the packet without IP header rather than forcibly add it for the sake of a trace.

Gustavo pointed out that there is not a clear winner one way or the other, so the convention above seems clear enough to me and we can document "if the header is passed alongside the packet, that means that the packet doesn't have an IPv4 header."  The key thing to me is that the traces are documented and internally consistent (in particular, m_dropTrace doesn't return different types of Packets depending on where it is invoked).

Any further comments?

(In reply to comment #2)
> 
> Gustavo pointed out that there is not a clear winner one way or the other, so
> the convention above seems clear enough to me and we can document "if the
> header is passed alongside the packet, that means that the packet doesn't have
> an IPv4 header."  The key thing to me is that the traces are documented and
> internally consistent (in particular, m_dropTrace doesn't return different
> types of Packets depending on where it is invoked).
> 
> Any further comments?
> 

+1

Created attachment 617 [details]
doc patch

I'm not very good at documentation, but I guess something along these lines should be enough...

Created attachment 620 [details]
a few more comments

Here are just a couple more comments and typo fixes with regard to this bug

(In reply to comment #4)
> Created an attachment (id=617) [details]
> doc patch
> 
> I'm not very good at documentation, but I guess something along these lines
> should be enough...
> 

+1.

I will try to find this again, but I noticed that one of the drop traces in CSMA had an inconsistent approach to including the mac header.  But I will open another bug if I find that again.  I think that the IP layer is consistent.