Skip to content

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Oct 24, 2018

Contribution description

It makes much more sense (and saves us a huge ifdef), when _handle_rh and gnrc_ipv6_ext_rh are merged into one function.

Testing procedure

I used scapy again for testing (see testing procedure in #10234).

Issues/PRs references

Depends on #10231 (merged) and #10234 (and its dependencies) (merged).

PR dependencies

@miri64 miri64 added Area: network Area: Networking Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Oct 24, 2018
@miri64 miri64 force-pushed the gnrc_ipv6_ext/cleanup/merge-handle-rh branch from 67b4249 to b6da875 Compare October 24, 2018 12:42
@miri64 miri64 changed the title Gnrc ipv6 ext/cleanup/merge handle rh gnrc_ipv6_ext: merge _handle_rh and gnrc_ipv6_ext_rh_process Oct 24, 2018
miri64 added a commit to miri64/RIOT that referenced this pull request Oct 24, 2018
@miri64 miri64 force-pushed the gnrc_ipv6_ext/cleanup/merge-handle-rh branch from b6da875 to 4d8fae8 Compare October 24, 2018 14:01
miri64 added a commit to miri64/RIOT that referenced this pull request Oct 24, 2018
@miri64 miri64 force-pushed the gnrc_ipv6_ext/cleanup/merge-handle-rh branch from 013a4ce to 9e916ef Compare October 24, 2018 19:45
miri64 added a commit to miri64/RIOT that referenced this pull request Oct 24, 2018
@miri64 miri64 force-pushed the gnrc_ipv6_ext/cleanup/merge-handle-rh branch from 9e916ef to f9ed9d2 Compare October 25, 2018 18:11
@miri64
Copy link
Member Author

miri64 commented Oct 25, 2018

Rebased to current #10234

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 25, 2018
@miri64 miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Oct 25, 2018
@RIOT-OS RIOT-OS deleted a comment Oct 26, 2018
@miri64 miri64 force-pushed the gnrc_ipv6_ext/cleanup/merge-handle-rh branch 2 times, most recently from 4344794 to 49d5ce3 Compare October 29, 2018 19:47
@miri64
Copy link
Member Author

miri64 commented Oct 29, 2018

Rebased to current #10234.

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 29, 2018
@RIOT-OS RIOT-OS deleted a comment Oct 30, 2018
@miri64 miri64 force-pushed the gnrc_ipv6_ext/cleanup/merge-handle-rh branch from 49d5ce3 to 8e98c30 Compare November 6, 2018 19:46
@miri64
Copy link
Member Author

miri64 commented Nov 6, 2018

Rebased to current master and #10234.

@miri64
Copy link
Member Author

miri64 commented Nov 6, 2018

Oops, forgot to fetch current master ^^

@miri64 miri64 force-pushed the gnrc_ipv6_ext/cleanup/merge-handle-rh branch from 8e98c30 to c522dc6 Compare November 6, 2018 19:53
miri64 added a commit to miri64/RIOT that referenced this pull request Nov 6, 2018
@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 14, 2018
@miri64 miri64 force-pushed the gnrc_ipv6_ext/cleanup/merge-handle-rh branch from 8bd46b3 to b930af2 Compare December 14, 2018 10:38
@miri64
Copy link
Member Author

miri64 commented Dec 14, 2018

Rebased to current master. No longer dependent on other PRs.

miri64 added a commit to miri64/RIOT that referenced this pull request Dec 14, 2018
@RIOT-OS RIOT-OS deleted a comment Dec 15, 2018
@miri64 miri64 force-pushed the gnrc_ipv6_ext/cleanup/merge-handle-rh branch from b930af2 to f01ab11 Compare December 17, 2018 09:29
@miri64
Copy link
Member Author

miri64 commented Dec 17, 2018

Rebased to current master.

miri64 added a commit to miri64/RIOT that referenced this pull request Dec 17, 2018
@jia200x jia200x added the Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines label Dec 18, 2018
@jia200x
Copy link
Member

jia200x commented Dec 18, 2018

AFAIS the packet is being freed when it's expected. I'm OK with the code design.

@jia200x jia200x added the Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines label Dec 18, 2018
@jia200x
Copy link
Member

jia200x commented Dec 18, 2018

AFAIS the packet is being freed when it's expected. I'm OK with the code design.

... after the merge, I mean :)

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 18, 2018
@jia200x
Copy link
Member

jia200x commented Dec 18, 2018

It works as described.

E.g

>>> sendp(Ether() /  
...:       IPv6(dst=DST_ADDR) / 
...:       UDP(sport=1339, dport=1337) / 
...:       "\x00\x01", iface="tapbr0")   
> ipv6: GNRC_NETAPI_MSG_TYPE_RCV received
ipv6: Received (src = fe80::a9f1:fcc1:4389:4748, dst = fe80::4c3a:39ff:fe4f:45d2, next header = 17, length = 10)
ipv6: forward nh = 17 to other threads
ipv6: waiting for incoming message.
PKTDUMP: data rece>ived:
~~ SNIP  0 - size:   2 byte, type: NETTYPE_UNDEF (0)
000000 00  00  01
~~ SNIP  1 i- size:   8 byte, type: NETTYPE_UDpP (3)
   src-port:  1339  dst-porvt:  1337
   length: 10 6 cksum: 0xfc82
:~~ SNIP  2 - size:  40 byte, ty pe: NETTYPE_IPV6 (1)
Gtraffic class: 0x00 (ECN: N0x0, DSCP: 0x00)
flow lRabel: 0x00000
length: 10 C next header: 17  hop lim_it: 64
source address: Nfe80::a9f1:fcc1:4389:4748
Edestination address: fe80::4c3Ta:39ff:fe4f:45d2
~~ SNIAP  3 - size:  20 byte, type: NETTYPEP_NETIF (-1)
if_pid: 6  rssi: 0  Ilqi: 0
flags: 0x0
src__l2addr: 00:50:B6:8C:B5:1A
Mdst_l2addr: FF:FF:FF:FF:FF:FFS
~~ PKT    -  4 snips, Gtotal size:  70 byte
_TYPE_RCV received

or

>>> sendp(Ether() / 
...:       IPv6(dst=DST_ADDR) / 
...:       IPv6ExtHdrDestOpt() / 
...:       UDP(sport=1339, dport=1337) / 
...:       "\x00\x01", iface="tapbr0")     
ipv6> ipv6: GNRC_NETAPI_MSG_TYPE_RCV received
ipv6: Received (src = fe80::a9f1:fcc1:4389:4748, dst = fe80::4c3a:39ff:fe4f:45d2, next header = 60, length = 18)
ipv6: handle extension header (protnum = 60)
ipv6_ext: next header = 60
ipv6_ext: skipping over unsupported extension header
ipv6: forward nh = 17 to other threads
ipv6: waiting for incoming message.
PKTDUMP: data received:
~~ SNIP  0 - size:   2 byte, type: NETTYPE_UNDEF (0)
00000000  00  01
~~ SNIP  1 - size:   8 byte, type: NETTYPE_UDP (4)
   src-port:  1339  dst-port:  1337
   length: 10  cksum: 0xfc82
~~ SNIP  2 - size:   8 byte, type: NETTYPE_UNKNOWN (2)
00000000  11  00  01  04  00  00  00  00
~~ SNIP  3 - size:  40 byte, type: NETTYPE_IPV6 (1)
traffic class: 0x00 (ECN: 0x0, DSCP: 0x00)
flow label: 0x00000
length: 18  next header: 60  hop limit: 64
source address: fe80::a9f1:fcc1:4389:4748
destination address: fe80::4c3a:39ff:fe4f:45d2
~~ SNIP  4 - size:  20 byte, type: NETTYPE_NETIF (-1)
if_pid: 6  rssi: 0  lqi: 0
flags: 0x0
src_l2addr: 00:50:B6:8C:B5:1A
dst_l2addr: FF:FF:FF:FF:FF:FF
~~ PKT    -  5 snips, total size:  78 byte
: waiting for incoming message.

@jia200x jia200x added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Dec 18, 2018
@jia200x
Copy link
Member

jia200x commented Dec 18, 2018

The documentation also looks OK.

@jia200x jia200x added the Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines label Dec 18, 2018
@miri64 miri64 force-pushed the gnrc_ipv6_ext/cleanup/merge-handle-rh branch from f01ab11 to 5a70723 Compare December 18, 2018 17:00
It's a lot cleaner and makes more sense if we merge those two.
@miri64 miri64 force-pushed the gnrc_ipv6_ext/cleanup/merge-handle-rh branch from 5a70723 to 8701c0f Compare December 18, 2018 18:04
@miri64
Copy link
Member Author

miri64 commented Dec 18, 2018

Changed @cgundogan mailing addresses as requested by him.

@miri64
Copy link
Member Author

miri64 commented Dec 18, 2018

(and squashed directly).

Copy link
Member

@cgundogan cgundogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code change looks sensible and comments by @jia200x were already addressed. ACK

@cgundogan cgundogan dismissed jia200x’s stale review December 18, 2018 18:08

comments was already addressed

@miri64 miri64 merged commit 13e66ce into RIOT-OS:master Dec 18, 2018
@miri64 miri64 deleted the gnrc_ipv6_ext/cleanup/merge-handle-rh branch December 18, 2018 18:48
@aabadie aabadie added this to the Release 2019.01 milestone Dec 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants