gnrc_sixlowpan_iphc_nhc: fix NHC UDP decoding for fragmented packets

[miri64]

While running my experiments for my masterthesis I realized that our NHC implementation isn't really able to handle fragmented packets. With this fix it does though it's really ugly.

[jnohlgard]

Nice!

[jnohlgard]

Tested working with a Contiki RPL border router connected to Linux, RIOT gnrc_networking example on mulle and using netcat on linux to send long strings to the RIOT node. Without this patch only strings less than 50-something chars could get through.

About the supposed uglyness, I'll leave that to someone more involved with the gnrc stack to decide.

[OlegHahm]

If this is working, I think we can live with it. I've seen uglier code.

[jnohlgard]

Somewhat related Q: Is there any risk of leaking memory when fragments get lost or are the partial packets freed somewhere?

[cgundogan]

@gebart obsolete fragments should be gc'ed here

[miri64]

Somewhat related Q: Is there any risk of leaking memory when fragments get lost or are the partial packets freed somewhere?

@Yonezawa-T2 already has a PR open for that: #4771.

[miri64]

Added a fix for compiling without NHC.

[cgundogan]

okay, will test with the fix a little bit later. And where is the [ci skip] in your fixup commit message? (;

[cgundogan]

still need to do some tests with a linux-based boarder router. I have no hardware around today, so I can do it tomorrow

[miri64]

And where is the [ci skip] in your fixup commit message? (;

I forward your complaints to the git developers since they still don't allow me to configure the prefix generated by git commit --fixup=<commit> and picked up by git rebase -i --autosquash ;-)

[miri64]

(Done)

[miri64]

((http://permalink.gmane.org/gmane.comp.version-control.git/288173))

[cgundogan]

((http://permalink.gmane.org/gmane.comp.version-control.git/288173))

👍 couldn't this be fixed with some pre-commit hooks?

[miri64]

Not if I want the commit still be picked up by --autosquash ;-)

[cgundogan]

I understand --autosquash as a mechanism that inspects the prefix and sets the action while rebasing. In a pre-commit hook you could juts append [ci skip] at the end of the commit message?

[miri64]

I can try, but as far as I undestand the prefixing is the other way around.

[cgundogan]

I can try, but as far as I undestand the prefixing is the other way around.

keep me informed, I would like to copy your fixup process (:

[miri64]

Nope. Appending or prepending [ci skip] to the commit message (either the original or the generated) will cause --autosquash not to pickup the commit. :(

[miri64]

</OT>

[miri64]

Last commit is a test of something someone proposed on the [edit: git] mailinglist.

[miri64]

(and removed: seemed to work).

[jnohlgard]

@cgundogan did you have a chance to test this today?

[miri64]

Done.

[cgundogan]

@gebart thanks for the reminder.
I just ran a test with a phytec wega board as a linux border router and a samr21-xpro running the microcoap example. I modified the coap handler to return a long lorem ipsum string.
I explicitely enabled gnrc_sixlowpan_iphc_nhc in the Makefile of that example.

https://www.cloudshark.org/captures/062253ad8832

@authmillenon looks correct?

[miri64]

Mh… I don't know your setup, but did you make sure that a RIOT node received the long coap packet? When it successfully received the packet it is correct. :-)

[cgundogan]

nope, the coap message from the linux side was very small (coap get). Only the response from riot -> linux was big (:

[miri64]

That explains why your previous tries were working ;-). This fix is for reception of fragmented NHC encoded packets. So if you are just send those with RIOT you do not test anything touched by this PR ;-)

[miri64]

An alternative test would be to use my patch in RIOT-OS/Release-Specs#10 (comment) to send 1 KiB sized packets between two RIOT nodes. @cgundogan just needed to go, so if someone else wants to test until monday: knock yourselves out ;-)

[miri64]

A friendly reminder for @cgundogan to test this tomorrow ;-)

[miri64]

@cgundogan ping?

[jnohlgard]

I am having a problem with the BR dropping the entire payload part of a forwarded UDP packet, possibly related to this PR. Currently investigating..

[miri64]

Related as in this PR fixes it or this PR causes it?

[miri64]

The fix for the border router NHC is at #4544 btw.

[jnohlgard]

@authmillenon Thanks for the hint, I'll try that PR

[cgundogan]

will test tomorrow - I promise (;

[miri64]

I'm waiting in anticipation :)

[cgundogan]

I don't say anything else, before I spread out more empty promises (:
Will be around noon at the university. If you are also there, maybe we can test/confirm it together.

[cgundogan]

tested together with @authmillenon between two RIOT nodes and also with a RIOT node with a linux node.
NHC breaks on master and works with this branch. ACK!

[cgundogan]

travis is happy, let's see what Murdock has to say.

[jnohlgard]

I'm still seeing one issue with NHC and multihop routed fragmented packets, but I don't think this PR is the cause

[cgundogan]

@gebart is there an issue somewhere describing this? Maybe I can also invest some time debugging that.

Nevertheless, travis && murdock are happy! GO

[jnohlgard]

@cgundogan I think the problem is that #4544 does not fix everything with regards to UDP NHC and fragmented packets