Created attachment 2257 [details]
test for patch

Chip, can we document this (somewhere) such as follows?

"Newer tcpdump implementations support the --time-stamp-precision option, and if the OS can deliver nanosecond timestamps, the dump will be stored with a different timestamp, and the file will have a different "magic number" to notify the program (tcpdump, Wireshark) that the format is different.  The ns3::PcapFileWrapper::NanosecMode attribute is used to change the timestamp resolution to nanoseconds from the default microseconds.  Not all programs are able to support the nanosecond format."

By default, when saving pcap packet trace files, ns3 records
packet timestamps with microsecond resolution. Newer implementations
of tcpdump, Wireshark and other	libpcap	based applications
support reading and writing pcap packet trace files that have
nanosecond resolution timestamps.

The boolean attribute ns3::PcapFileWrapper::NanosecMode may
be set to true to change the resolution of timestamp written
to pcap	tracefiles.

The following example C++ code shows how to do this:

  Config::SetDefault ("ns3::PcapFileWrapper::NanosecMode", 
                      BooleanValue (true));                                                                       

Timestamps for saving pcap trace files are created by taking the
value of ns3::Simulator::Now() and converting to either
microseconds or nanoseconds with the .GetMicroSeconds() or
.GetNanoSeconds() method.


The pcap packet trace files have "magic number" at the to notify
programs that read the files that the timestamp format is
different.  The format of the pcap capture files is more fully
documented at:

  http://www.tcpdump.org/manpages/pcap-savefile.5.html

Note: This attribute does not change the way that ns3 interprets
      timestamps when *reading* pcap trace files because the correct
      interpretation of these timestamps is determined by the 
      "magic number" read from the file header.

Hi Chipp,

I'm ok with the patch, but I have a doubt.
When we read a file, since the PCAP header (including the magic number) is read when the file is opened (Open function), why shouldn't we remember if the file is in nano or micro seconds ?

Other than that, no comments

Thomasso, you are right -- when reading a PCAP file, the magic number read from the file header will control the timestamp format. The new attribute has no effect on this.

However, when writing a PCAP file, the timestamps can either have microsecond or nanosecond resolution. The boolean attribute ns3::PcapFileWrapper::NanosecMode allows a user to choose whether timestamps are written with nanosecond resolution (attribute set to true) or microsecond resolution (attribute set to false).

Since a minimum length Ethernet packet takes less than 1 microsecond to send out a NetDevice at 1 Gigabit, having nanosecond resolution allows verification of proper implementation of these NetDevices. (A minimum length packet is 64 Bytes, which is is 512 nanoseconds at 1 Gbps. Similarly, a maximum length packet [1518 bytes] takes about 12.1 microseconds.)

My comment is about this statement you made:

Note: This attribute does not change the way that ns3 interprets
      timestamps when *reading* pcap trace files because the correct
      interpretation of these timestamps is determined by the 
      "magic number" read from the file header.

You *can* change the way ns-3 is reading PCAPs. All you have to do is to read the magic number in the Open and ReadAndVerifyFileHeader functions.

Chip, can you please propose a revised patch to address Tommaso's concern with how it is documented?

Actually my concern was on the comment. The patch is "just" code, it doesn't actually modifies the documentation.
No need to change what's not changed.

The only things to fix are:
1) src/network/utils/pcap-file.h - undocumented variable "m_nanosecMode"
2) Some comments "CWEBB" - are they necessary ?

Other than this, I have no other concerns.

(In reply to Tom Henderson from comment #7)
> Chip, can you please propose a revised patch to address Tommaso's concern
> with how it is documented?

I understand Thomasso's comment about the documentation needing clarification, and I understand the comment about needing to document the m_nanosecMode variable in cap-file.h. 

So I will update the patch to fix these concerns..

However, I don't understand the other concern about documentation:

   You *can* change the way ns-3 is reading PCAPs. All you have to do 
   is to read the magic number in the Open and ReadAndVerifyFileHeader 
   functions.

Maybe we're splitting hairs here or we're agreeing violently :-). When opening a pcap file for reading, PcapFile::Open calls ReadAndVerifyHeader, and this method sets the PcapFile::m_nanosecMode member value to true or false based on the file's magic number. Since it is a member value, it is saved for the rest of the time that packets are read from the file. The ns3 attribute "ns3::PcapFileWrapper::NanosecMode" doesn't change how the magic numbers are interpreted in ReadAndVerifyHeader for input files.

So, what I meant by: 

   Note: This attribute does not change the way that ns3 interprets
         timestamps when *reading* pcap trace files because the correct
         interpretation of these timestamps is determined by the 
         "magic number" read from the file header.

Is that the *ns3 attribute* "ns3::PcapFileWrapper::NanosecMode" does not 
change the way that timestamps are interpreted when reading from pcap files.

Perhaps it would be clearer to explicitly state what is meant by "This attribute"?

Would that help?

Hi Chip,

we're violently agreeing.

The attribute is only there for writing files, because *how* the file is read (nanoseconds or milliseconds) is decided by the magic number and the attribute has no effect.

Feel free to push this.

Cheers,

T.



(In reply to Chip Webb from comment #9)
> I understand Thomasso's comment about the documentation needing
> clarification, and I understand the comment about needing to document the
> m_nanosecMode variable in cap-file.h. 
> 
> So I will update the patch to fix these concerns..
> 
> However, I don't understand the other concern about documentation:
> 
>    You *can* change the way ns-3 is reading PCAPs. All you have to do 
>    is to read the magic number in the Open and ReadAndVerifyFileHeader 
>    functions.
> 
> Maybe we're splitting hairs here or we're agreeing violently :-). When
> opening a pcap file for reading, PcapFile::Open calls ReadAndVerifyHeader,
> and this method sets the PcapFile::m_nanosecMode member value to true or
> false based on the file's magic number. Since it is a member value, it is
> saved for the rest of the time that packets are read from the file. The ns3
> attribute "ns3::PcapFileWrapper::NanosecMode" doesn't change how the magic
> numbers are interpreted in ReadAndVerifyHeader for input files.
> 
> So, what I meant by: 
> 
>    Note: This attribute does not change the way that ns3 interprets
>          timestamps when *reading* pcap trace files because the correct
>          interpretation of these timestamps is determined by the 
>          "magic number" read from the file header.
> 
> Is that the *ns3 attribute* "ns3::PcapFileWrapper::NanosecMode" does not 
> change the way that timestamps are interpreted when reading from pcap files.
> 
> Perhaps it would be clearer to explicitly state what is meant by "This
> attribute"?
> 
> Would that help?

Created attachment 2299 [details]
Revised patches to support nanosecond timestamps in pcap files: pcap-file.cc, pcap-file.h pcap-file-wrapper.cc pcap-file-wrapper.h

This revised patch makes four changes to the previous patch to addres Thomasso's comments raised during review:

   Adds missing documentation for member variable
   Removes two comments that asked questions about existing code

Here is revised documentation:

--------------

By default, when saving pcap packet trace files, ns3 records
packet timestamps with microsecond resolution. Newer implementations
of tcpdump, Wireshark and other	libpcap	based applications
support reading and writing pcap packet trace files that have
nanosecond resolution timestamps.

The boolean attribute ns3::PcapFileWrapper::NanosecMode may
be set to true to change the resolution of timestamp written
to pcap	tracefiles.

The following example C++ code shows how to do this:

  Config::SetDefault ("ns3::PcapFileWrapper::NanosecMode", 
                      BooleanValue (true));                                                                       

Timestamps for saving pcap trace files are created by taking the
value of ns3::Simulator::Now() and converting to either
microseconds or nanoseconds with the .GetMicroSeconds() or
.GetNanoSeconds() method.

Pcap packet trace files have "magic number" in the file header.
One function of this magic number is to indicate whether the
packet timestamps in that file have nanosecond or microsecond 
resolution. The format of the pcap capture files is more 
fully documented at:

  http://www.tcpdump.org/manpages/pcap-savefile.5.html

I removed two comments from the patch that asked questions. I ask them here in more detail:

Question #1) Should the PcapFileWrapper object have a DoDispose method? I am not sure about this, but I thought that objects derived from ns3::Object class needed DoDispose methods. And I wondered if this was related to another reported bug for flushing files. Here's the documentation I read about DoDispose that led to this question:

    void Object::DoDispose (void)	

    This method is called by Dispose() or by the Object's destructor, 
    whichever comes first.

       Subclasses are expected to implement their real destruction code 
       in an overriden version of this method and chain up to their 
       parent's implementation once they are done. i.e, for simplicity, 
       the destructor of every subclass should be empty and its content 
       should be moved to the associated DoDispose() method.

Question #2) Should the use of RegisterStream depend upon whether the pcap file stream is input or output? Right now, all pcap files (input or output) are registered, but I think it only matters for output streams.

Both of these questions might be related to Bug #2164.

(In reply to Chip Webb from comment #13)
> I removed two comments from the patch that asked questions. I ask them here
> in more detail:
> 
> Question #1) Should the PcapFileWrapper object have a DoDispose method? I am
> not sure about this, but I thought that objects derived from ns3::Object
> class needed DoDispose methods. And I wondered if this was related to
> another reported bug for flushing files. Here's the documentation I read
> about DoDispose that led to this question:
> 
>     void Object::DoDispose (void)	
> 
>     This method is called by Dispose() or by the Object's destructor, 
>     whichever comes first.
> 
>        Subclasses are expected to implement their real destruction code 
>        in an overriden version of this method and chain up to their 
>        parent's implementation once they are done. i.e, for simplicity, 
>        the destructor of every subclass should be empty and its content 
>        should be moved to the associated DoDispose() method.

It probably should have, in keeping with the architecture.  However, it may not matter much in this case, since the private data doesn't contain pointers to other objects.  This is mainly used to break reference cycles that prevent heap-allocated objects from being deleted at the end of simulation, so that the valgrind report is clean.  

> 
> Question #2) Should the use of RegisterStream depend upon whether the pcap
> file stream is input or output? Right now, all pcap files (input or output)
> are registered, but I think it only matters for output streams.

I think you are correct, but it doesn't matter that input streams are registered.

> 
> Both of these questions might be related to Bug #2164.

I don't think question 2 is related since it pertains to flushing due to fatal errors.  Bug 2164 pertains to file handles not being closed/flushed when the program is ending normally, due to (possibly) destructors not being called (although I haven't looked into it in detail).

thanks for the clarifications. It helps me understand :-)

pushed in 11930:471023dc0c7e