Adds cooked header export

Adds cooked header export

In order for DCE to be able to export netlink packets in a format
readable by wireshark, one needs to prepend a cooked header to the
netlink packet. This commit just does that when exporting netlink traces.

BUG=

[Tom Henderson, 2015-12-14 03:10:33]

Please change the following:

1) place sll-header files in src/network/utils rather than src/network/model

2) please align to ns-3 coding style:

- space before opening parentheses
- braces/indentation is as follows:

if (condition)
  {
    statement;
  }
else
  {
    statement;
  }

https://codereview.appspot.com/276320043/diff/1/src/network/model/sll-header.h
File src/network/model/sll-header.h (right):

https://codereview.appspot.com/276320043/diff/1/src/network/model/sll-header....
src/network/model/sll-header.h:38: \see
http://www.tcpdump.org/linktypes/LINKTYPE_LINUX_SLL.html
another useful URL:  https://wiki.wireshark.org/SLL

https://codereview.appspot.com/276320043/diff/1/src/network/model/sll-header....
src/network/model/sll-header.h:93: \return Packet type
add * to these Doxygen lines before the \param or \return, such as 

  /**
   * \return Packet type
   */

(also below in a few spots)

[mattator, 2015-12-15 00:44:34]

I pushed an update which should address your comments.
In fact, I mainly applied check-style.py -i to the files, but there is
a strange behavior, it keeps removing accents, such that "Université"
becomes "Universit".

Also wonder if anyone has experience setting check-style as a
precommit hook in git ? I tried creating one but if there are some
already available :)



2015-12-14 4:10 GMT+01:00  <tomh.org@gmail.com>:
> Please change the following:
>
> 1) place sll-header files in src/network/utils rather than
> src/network/model
>
> 2) please align to ns-3 coding style:
>
> - space before opening parentheses
> - braces/indentation is as follows:
>
> if (condition)
>   {
>     statement;
>   }
> else
>   {
>     statement;
>   }
>
>
>
>
https://codereview.appspot.com/276320043/diff/1/src/network/model/sll-header.h
> File src/network/model/sll-header.h (right):
>
>
https://codereview.appspot.com/276320043/diff/1/src/network/model/sll-header....
> src/network/model/sll-header.h:38: \see
> http://www.tcpdump.org/linktypes/LINKTYPE_LINUX_SLL.html
> another useful URL:  https://wiki.wireshark.org/SLL
>
>
https://codereview.appspot.com/276320043/diff/1/src/network/model/sll-header....
> src/network/model/sll-header.h:93: \return Packet type
> add * to these Doxygen lines before the \param or \return, such as
>
>   /**
>    * \return Packet type
>    */
>
> (also below in a few spots)
>
> https://codereview.appspot.com/276320043/

[Tom Henderson, 2015-12-15 01:00:51]

On 2015/12/15 00:44:34, mattator wrote:
> I pushed an update which should address your comments.
> In fact, I mainly applied check-style.py -i to the files, but there is
> a strange behavior, it keeps removing accents, such that "Université"
> becomes "Universit".
> 
> Also wonder if anyone has experience setting check-style as a
> precommit hook in git ? I tried creating one but if there are some
> already available :)
> 

I do not recommend using a commit hook for it to try to automate it; you will
quickly run into cases where you either are blocked from committing because of
unrelated style problems elsewhere in the file, or you will be forced to change
all of these style problems elsewhere, in which case your commit will have a lot
of unrelated whitespace changes.  

We do not want constant churn of our whitespace by continually trying to update
our files, so that we don't break out-of-tree code when we don't have to.  If we
were always updating, we would see a lot of churn because the underlying tool
(uncrustify) itself evolves over time and people are using different versions. 
Instead, we aim for the following:

- when you add new files to ns-3-dev for the first time, it is good practice to
run check-style.py on them so that the are reasonably clean and conformant at
the onset
- if you are surgically touching pieces of existing files, it is better practice
to just limit the scope of your style changes to the code you touched, which
usually means not running check-style.py but fixing it manually

[mattator, 2015-12-16 10:18:13]

Agreed. Ideally one could run the check-style only on parts that differ with the
master branch butthat may be complex.

When I said I mainly ran unscrutify, don't misunderstand me I also tried to
address your other comments so it's ready for review.


On 2015/12/15 01:00:51, Tom Henderson wrote:
> On 2015/12/15 00:44:34, mattator wrote:
> > I pushed an update which should address your comments.
> > In fact, I mainly applied check-style.py -i to the files, but there is
> > a strange behavior, it keeps removing accents, such that "Université"
> > becomes "Universit".
> > 
> > Also wonder if anyone has experience setting check-style as a
> > precommit hook in git ? I tried creating one but if there are some
> > already available :)
> > 
> 
> I do not recommend using a commit hook for it to try to automate it; you will
> quickly run into cases where you either are blocked from committing because of
> unrelated style problems elsewhere in the file, or you will be forced to
change
> all of these style problems elsewhere, in which case your commit will have a
lot
> of unrelated whitespace changes.  
> 
> We do not want constant churn of our whitespace by continually trying to
update
> our files, so that we don't break out-of-tree code when we don't have to.  If
we
> were always updating, we would see a lot of churn because the underlying tool
> (uncrustify) itself evolves over time and people are using different versions.

> Instead, we aim for the following:
> 
> - when you add new files to ns-3-dev for the first time, it is good practice
to
> run check-style.py on them so that the are reasonably clean and conformant at
> the onset
> - if you are surgically touching pieces of existing files, it is better
practice
> to just limit the scope of your style changes to the code you touched, which
> usually means not running check-style.py but fixing it manually

[Tommaso Pecorella, 2015-12-26 10:57:15]

I don't have many things to say about this patch (the code), but I do have a
couple of general comments.

1) There's a typo in trace-helper.h: DLT_LINUX_SSL should be DLT_LINUX_SLL (line
53).

2) A general objection / curiosity.
NetLink is one thing, SLL another one. Wireshark (or better, the network capture
part) uses DLT_LINUX_SLL to write NetLink (and PPP) packets to the .pcap.
We write .pcap files "manually", and Wireshark, which is a simple fronted, can
like them or not.
My question is: should we add the SLL header to nettling or it should be more
correct to use SLL *instead* of NetLink ?

Unfortunately I'm not using DCE so I can't check how NetLink is used, but it
could be something to think about.

[mattator, 2016-01-05 15:19:16]

Thanks for the comments tommaso. I put this patch on hold: I've sent a 
patch to DCE and after discussing with hajime will come back to this patch 
and address your comments.
Here is the PR https://github.com/direct-code-execution/ns-3-dce/pull/23 
and it generates for instance this netlink pcap:
https://transfer.sh/ohADQ/netlink-test.pcap

i went to #wireshark to try displaying the netlink without the SLL headers 
but that seemed impossible. Even if it were possible (with wireshark or 
another), I believe it's worth making it readable out of the box. 

In fact what I tried to do with this patch was to spare the deep copy of a 
packet necessary to prepend the SLL header via modifying ns3 but netlink 
communications are low rate, thus a deep copy should not be a problem and 
will be more straightforward. I will try to implement a sll+netlink DCE 
exporter that does not modify ns3.

Regards

Le samedi 26 décembre 2015 11:57:16 UTC+1, Tommaso Pecorella a écrit :
>
> I don't have many things to say about this patch (the code), but I do 
> have a couple of general comments. 
>
> 1) There's a typo in trace-helper.h: DLT_LINUX_SSL should be 
> DLT_LINUX_SLL (line 53). 
>
> 2) A general objection / curiosity. 
> NetLink is one thing, SLL another one. Wireshark (or better, the network 
> capture part) uses DLT_LINUX_SLL to write NetLink (and PPP) packets to 
> the .pcap. 
> We write .pcap files "manually", and Wireshark, which is a simple 
> fronted, can like them or not. 
> My question is: should we add the SLL header to nettling or it should be 
> more correct to use SLL *instead* of NetLink ? 
>
> Unfortunately I'm not using DCE so I can't check how NetLink is used, 
> but it could be something to think about. 
>
> https://codereview.appspot.com/276320043/ 
>

[Tommaso Pecorella, 2016-01-05 17:14:44]

Hi Matt,

perhaps I wasn't clear. I don't mind at all the SLL proposal. Indeed, I *like*
it.
My concern is: don't do it *instead of* NetLink, do it *in addition to* NetLink.

As is: instead of a "if(file->GetDataLinkType() == PcapHelper::DLT_NETLINK)",
what about adding another "if(file->GetDataLinkType() == PcapHelper::DLT_SLL)" ?
Another alternative could be a flag to be set when you open the pcap trace.
I'm not really picky about how to do it, but if the user asks for a bare-bone
NetLink, he/she should be able to have it... with the obvious consequences
(i.e., WireShark will not read it).

Cheers,

T.

PS: I'm gonna fix the SLL / SSL typo.

[mattator, 2016-01-17 22:01:18]

I asked wireshark folks to deal with this and they fixed it this we \o/ 
https://code.wireshark.org/review/#/c/13335/ 

We can now write encapsulate netlink packets with Sll headers in a pcap 
with the correct SLL data type and wireshark will decode them. 
Modifying trace-helper.cc was a way to access the file->Write 
(Simulator::Now (), sll, p); function to prevent a copy of the packet when 
prepending the sll-header. I don't believe it's worth adding a special case 
in ns3 to handle that, hence I will generate the SLL headers from within 
DCE and prepend them to netlink packets. 
In the end I just need DCE to be able to access the sll-header.* files. 
Now the question is: should the sll-header files be provided by DCE or ns3 
knowing that ns3 vanilla wouldn't use them (for now) ? 


Le mardi 5 janvier 2016 18:14:45 UTC+1, Tommaso Pecorella a écrit :
>
> Hi Matt, 
>
> perhaps I wasn't clear. I don't mind at all the SLL proposal. Indeed, I 
> *like* it. 
> My concern is: don't do it *instead of* NetLink, do it *in addition to* 
> NetLink. 
>
> As is: instead of a "if(file->GetDataLinkType() == 
> PcapHelper::DLT_NETLINK)", what about adding another 
> "if(file->GetDataLinkType() == PcapHelper::DLT_SLL)" ? 
> Another alternative could be a flag to be set when you open the pcap 
> trace. 
> I'm not really picky about how to do it, but if the user asks for a 
> bare-bone NetLink, he/she should be able to have it... with the obvious 
> consequences (i.e., WireShark will not read it). 
>
> Cheers, 
>
> T. 
>
> PS: I'm gonna fix the SLL / SSL typo. 
>
> https://codereview.appspot.com/276320043/ 
>

[Hajime Tazaki, 2016-01-18 01:10:13]

On 2016/01/17 22:01:18, mattator wrote:

> Now the question is: should the sll-header files be provided by DCE or ns3 
> knowing that ns3 vanilla wouldn't use them (for now) ? 

I think it's worth to include in ns-3-dev (not in DCE) as the SLL header can be
used with general purposes.

[mattator, 2016-01-18 11:47:22]

On 2016/01/18 01:10:13, Hajime Tazaki wrote:
> On 2016/01/17 22:01:18, mattator wrote:
> 
> > Now the question is: should the sll-header files be provided by DCE or ns3 
> > knowing that ns3 vanilla wouldn't use them (for now) ? 
> 
> I think it's worth to include in ns-3-dev (not in DCE) as the SLL header can
be
> used with general purposes.

I just updated the patch discarding changes to trace-helper.cc .

[Tommaso Pecorella, 2016-01-18 15:21:25]

On 2016/01/18 11:47:22, mattator wrote:
> On 2016/01/18 01:10:13, Hajime Tazaki wrote:
> > On 2016/01/17 22:01:18, mattator wrote:
> > 
> > > Now the question is: should the sll-header files be provided by DCE or ns3

> > > knowing that ns3 vanilla wouldn't use them (for now) ? 
> > 
> > I think it's worth to include in ns-3-dev (not in DCE) as the SLL header can
> be
> > used with general purposes.
> 
> I just updated the patch discarding changes to trace-helper.cc .

Why not adding a "if(file->GetDataLinkType() == PcapHelper::DLT_LINUX_SLL)" ?

Other than that, +1

T.

[mattator, 2016-01-18 15:36:53]

I believe it's cleaner to generate the SLL header from where it's added
because:
- that's where you know more stuff, for instance in trace-helper we
assume packet type is always SllHeader::UNICAST_FROM_PEER_TO_ME
sll.SetPacketType(SllHeader::UNICAST_FROM_PEER_TO_ME); but we have no
way to know it's true.
- we should not add specific cases otherwise user won't understand why
in one case it prepends an header and not in the other case.

Ideally one should be able to hook to
static void SinkWithHeader (Ptr<PcapFileWrapper> file, const Header&
header, Ptr<const Packet> p);
instead of
static void DefaultSink (Ptr<PcapFileWrapper> file, Ptr<const Packet> p);
but right now there is no way to do: SinkWithHeader is never used
because it can't be used (private).

In one of my branch, I had improved callbacks so that it could detect
which sink to use but it's too complex to upstream and might be
obsolete if we upgrade callbacks to c++11.

2016-01-18 16:21 GMT+01:00  <tommypec@gmail.com>:
> On 2016/01/18 11:47:22, mattator wrote:
>>
>> On 2016/01/18 01:10:13, Hajime Tazaki wrote:
>> > On 2016/01/17 22:01:18, mattator wrote:
>> >
>> > > Now the question is: should the sll-header files be provided by
>
> DCE or ns3
>>
>> > > knowing that ns3 vanilla wouldn't use them (for now) ?
>> >
>> > I think it's worth to include in ns-3-dev (not in DCE) as the SLL
>
> header can
>>
>> be
>> > used with general purposes.
>
>
>> I just updated the patch discarding changes to trace-helper.cc .
>
>
> Why not adding a "if(file->GetDataLinkType() ==
> PcapHelper::DLT_LINUX_SLL)" ?
>
> Other than that, +1
>
> T.
>
> https://codereview.appspot.com/276320043/

[Tommaso Pecorella, 2016-01-18 16:07:06]

On 2016/01/18 15:36:53, mattator wrote:
> I believe it's cleaner to generate the SLL header from where it's added
> because[...]

You're right.

+1 with no changes.

T.