Bug 1557

Summary: errors in function OpenFlowSwitchNetDevice::BufferFromPacket()
Product: ns-3 Reporter: Long Li <lilong>
Component: openflowAssignee: Tom Henderson <tomh>
Status: RESOLVED FIXED    
Severity: normal CC: ns-bugs, ovidiu.poncea, tomh
Priority: P3    
Version: ns-3.15   
Hardware: PC   
OS: Linux   
Attachments: we
a second proposal, not that clean but may be faster when processed.
update Long Li's patch based on Packet::RemoveHeader()
updated patch without EthernetHeader peeking
updated patch without EthernetHeader peeking v2

Description Long Li 2012-12-17 08:34:09 EST
Created attachment 1489 [details]
we

In this function, it parse headers of each layer, this is ip header, tcp header, etc. But it uses PeekHeader function to extract the headeer without remove this header after parsing this header. So, in the next parse, the process starts from the same very start point. In the correct way, we should remove the parsed header.
The second problem is that, the packet pass to the function is start from ip layer without ethernet layer, so there is no need to remove the ethernet header.
Comment 1 Long Li 2012-12-17 08:35:19 EST
we should save the parameter packet first, for the function will modify the content of the structure.
Comment 2 Ovidiu Poncea 2015-08-22 12:44:25 EDT
Created attachment 2122 [details]
a second proposal, not that clean but may be faster when processed.

To fix this we either:
1. create a copy of the packet and work through the headers with RemoveHeader(), as done in ipcs-cassifier.cc IpcsClassifier::Classify, but this will add some overhead as each packet need to be copied. We need to copy it as BufferFromPacket is not supposed to alter the packet. [first attachment by Long Li]
2. Extend PeekHeader to search for the header start inside of a packet - this is rather complex and will affect many of the existing PeekHeader uses.
3. Create a short fragment with only the required header and use PeekHeader as before - this should be faster than creating a full copy of the packet. [this attachment]

The first solution looks better but since I already have this and spend some time dealing with it ... here it is.
Comment 3 Tom Henderson 2015-08-31 17:01:59 EDT
Created attachment 2131 [details]
update Long Li's patch based on Packet::RemoveHeader()

I made the argument to this method a const Packet, then copied.

I believe that Long Li's patch was missing a RemoveHeader() for the EthernetHeader, so I added that.

Finally, I do not believe that CopyData() statement needs to be touched.

The tests pass with this patch; OK to commit?
Comment 4 Ovidiu Poncea 2015-09-01 07:47:14 EDT
Yep, it passes testing with a single modification: It seems that removing the EthernetHeader is not a good idea:

assert failed. cond="(verIhl >> 4) == 4", file=../src/internet/model/ipv4-header.cc, line=430

The reason is that the packet passed to the function it's actually an Ip packet, not an Ethernet packet so the header can'd be removed. 
But even though the packet it's an ip packet the test "if (packet->PeekHeader (eth_hd))" is used and is successful... don't know why and don't have time to look into it.

I also agree that CopyData() does not need to be touched.
Comment 5 Tom Henderson 2015-09-01 09:26:17 EDT
(In reply to Ovidiu Poncea from comment #4)
> Yep, it passes testing with a single modification: It seems that removing
> the EthernetHeader is not a good idea:
> 
> assert failed. cond="(verIhl >> 4) == 4",
> file=../src/internet/model/ipv4-header.cc, line=430

Is this reproducible with the current tests/examples?

> 
> The reason is that the packet passed to the function it's actually an Ip
> packet, not an Ethernet packet so the header can'd be removed. 
> But even though the packet it's an ip packet the test "if
> (packet->PeekHeader (eth_hd))" is used and is successful... don't know why
> and don't have time to look into it.
> 

I would like to fix this issue now.  We may be getting lucky that it is not asserting.

Please try the new patch that removes the EthernetHeader handling altogether.
Comment 6 Tom Henderson 2015-09-01 09:27:12 EDT
Created attachment 2132 [details]
updated patch without EthernetHeader peeking
Comment 7 Ovidiu Poncea 2015-09-01 12:26:33 EDT
Created attachment 2135 [details]
updated patch without EthernetHeader peeking v2

The Ethernet header picking can safely be removed, but the code inside the "if" need to be kept. In the original code, Packet->PeekHeader (eth_hd) will always return something != 0 so the code inside the if will always be executed.

This passes both the openflow test suite, the openflow-example and my own test suite.
Comment 8 Tom Henderson 2015-09-01 13:11:37 EDT
(In reply to Ovidiu Poncea from comment #7)
> Created attachment 2135 [details]
> updated patch without EthernetHeader peeking v2
> 
> The Ethernet header picking can safely be removed, but the code inside the
> "if" need to be kept. In the original code, Packet->PeekHeader (eth_hd) will
> always return something != 0 so the code inside the if will always be
> executed.
> 
> This passes both the openflow test suite, the openflow-example and my own
> test suite.


Thank you, this makes more sense to me now.  I'll move forward with this patch.
Comment 9 Ovidiu Poncea 2015-09-01 13:18:19 EDT
My pleasure helping with it.
Comment 10 Tom Henderson 2015-09-03 01:59:19 EDT
committed in 11626:6174db6ec156