Add (un)stripdsr

[hacklschorsch]

These elements allow fiddling with the payload of DSR data packets, by stripping / saving away the DSR header and later reversing the process.

[kohler]

Hey, a little note on this. Strip... elements ONLY move the packet header pointer, they don't change the underlying data. But this one assumes IP underneath, depends on the VLAN_ANNO in an odd way (it is best not to reuse annotation names for unrelated purposes), and also calculates a checksum, etc. Seems odd. Now, this is not a huge deal, I certainly don't maintain the Grid elements actively or know how they are used. But I wanted to point out to you this oddity, and ask if maybe there are two elements here combined into one, or the name wants to be something other than Strip/Unstrip. If you have thought about this previously, OK. What do you think?

[hacklschorsch]

Hi!

Thanks, I did not know that (un)strip elements should not modify data.

I made the elements mostly from the "DSRRouteTable::strip_headers()" method in dsrroutetable.cc. I merely added changes to make sure that the DSR part of the header is saved to some unused space instead of thrown away.

VLAN_ANNO: You're right. Maybe better if I made this explicit by, say, having a
#define DSRH_ANNO_OFFSET VLAN_ANNO_OFFSET
in the code and warning about this in the header (== man page)?

About the element assuming IP underneath: As DSR also assumes IP underneath I think that's OK. The DSR header is, for the most part, an addition of a source route to the IP header (the IP and DSR headers are intertwined, if you will). The calculation of checksums also stems from this (The DSR checksum field == the IP checksum field). Thus, the DSR header cannot be stripped away by moving a pointer. If one wants to be able to reconstruct the original IP packet, that is. If the inner payload is wanted, one could still add a StripIPHeader after the StripDSRHeader.

For the above reasons, I would go with making the using of the VLAN_ANNO field explicit and also adding the fact that this element modifies the packet data (not only metadata) to the documentation of this element. If find the name "(Un)StripDSRHeader" still fitting.

Thanks for your consideration,

Florian

[kohler]

Hi Florian!

Thanks for your explanations about the issues. I understand the strip/unstrip behavior much better now; seems reasonable. Having looked again, I've taken your current patch as is. Let me explain some issues I have with the patch, and then why I took the patch without changes.

1. The code only works if the IP header has no options.
2. Length checks are missing -- if a packet is too short Unstrip/Strip will get quite confused.
3. The UnstripDSRHeader element's header comment does not describe the funny behavior that happens with VLAN_ANNO==0 or 1.

A couple more nits like this.

But all this said, the fact is that you and your group are probably some of the few people who are using the elements/grid collection. It would be counterproductive to hold you to some ridiculous "quality standard" that the existing Grid elements don't follow anyway!!! You find these elements useful so having them in is better than not. The comments above may be useful for you, and I'll be happy to accept updates to the elements.

Thanks for your contributions!
Eddie