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 an attachment (id=617) [details]
doc patch

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

Created an attachment (id=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] [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.