Skip to content

gnrc_ipv6_ext_rh: add ICMPv6 error message sending #10242

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jan 3, 2019
Merged

gnrc_ipv6_ext_rh: add ICMPv6 error message sending #10242

merged 2 commits into from
Jan 3, 2019

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Oct 24, 2018 •
edited
Loading

Contribution description

This adds ICMPv6 error messages to the routing header handling of GNRC.

Testing procedure

Again, I used scapy to inject several broken RPL source routing headers packets (see testing procedure in #10234):

sendp(Ether() /
      IPv6(dst=DST_ADDR) /
      IPv6ExtHdrRouting(type=3, segleft=1))
sendp(Ether() /
      IPv6(dst=DST_ADDR) /
      IPv6ExtHdrRouting())

Issues/PRs references

Depends on #8594 (merged), #10227, and #10238 (and their dependencies). merged

Accompanies #8594 but isn't dependent on it (though that one fixes a lot of issues in gnrc_icmpv6_error.

PR dependencies

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking labels Oct 24, 2018
@miri64 miri64 requested a review from bergzand October 24, 2018 15:01
@miri64 miri64 force-pushed the gnrc_ipv6_ext_rh/enh/icmpv6_error_msg branch 2 times, most recently from caae194 to dd965e6 Compare October 25, 2018 18:26
@miri64
Copy link
Member Author

miri64 commented Oct 25, 2018

Rebased to current #10227 and #10238.

@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_rh/enh/icmpv6_error_msg branch from dd965e6 to af72b6d Compare October 29, 2018 19:52
@miri64
Copy link
Member Author

miri64 commented Oct 29, 2018

Rebased to current master, #10227, and #10238.

@RIOT-OS RIOT-OS deleted a comment Oct 30, 2018
@miri64 miri64 force-pushed the gnrc_ipv6_ext_rh/enh/icmpv6_error_msg branch from af72b6d to f9d718b Compare November 6, 2018 19:57
@miri64
Copy link
Member Author

miri64 commented Nov 6, 2018

Rebased to current master, #10227, and #10238.

@RIOT-OS RIOT-OS deleted a comment Nov 6, 2018
@miri64 miri64 force-pushed the gnrc_ipv6_ext_rh/enh/icmpv6_error_msg branch from f9d718b to 0934507 Compare November 8, 2018 17:48
@miri64
Copy link
Member Author

miri64 commented Nov 8, 2018

Rebased to current master, #10227, and #10238.

@RIOT-OS RIOT-OS deleted a comment Nov 9, 2018
@miri64 miri64 mentioned this pull request Nov 13, 2018
Merged
3 tasks
@miri64 miri64 added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Nov 13, 2018
@miri64 miri64 force-pushed the gnrc_ipv6_ext_rh/enh/icmpv6_error_msg branch 2 times, most recently from cb6d226 to 56dde45 Compare November 14, 2018 15:59
@miri64
Copy link
Member Author

miri64 commented Nov 14, 2018

Rebased to current master and dependencies (and added #8594 as a dependency and updated accordingly).

miri64 added a commit to miri64/RIOT that referenced this pull request Nov 14, 2018
@miri64
fixup! unittests/tests-rpl_srh: add failure tests
b1d90c3 
Adapt for RIOT-OS#10242
miri64 added a commit to miri64/RIOT that referenced this pull request Nov 14, 2018
@miri64
fixup! unittests/tests-rpl_srh: rename to tests-gnrc_rpl_srh
c815f32 
Adapt for RIOT-OS#10242
@miri64 miri64 mentioned this pull request Nov 14, 2018
Merged
@RIOT-OS RIOT-OS deleted a comment Nov 15, 2018
@miri64 miri64 force-pushed the gnrc_ipv6_ext_rh/enh/icmpv6_error_msg branch from 56dde45 to 316d93a Compare November 17, 2018 15:56
@miri64
Copy link
Member Author

miri64 commented Nov 17, 2018

Rebased to current dependencies.

@RIOT-OS RIOT-OS deleted a comment Nov 17, 2018
miri64 added a commit to miri64/RIOT that referenced this pull request Nov 17, 2018
@miri64
fixup! unittests/tests-rpl_srh: add failure tests
c99eab0 
Adapt for RIOT-OS#10242
@miri64
Copy link
Member Author

miri64 commented Dec 18, 2018

Rebased to current master.

@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 State: waiting for other PR State: The PR requires another PR to be merged first labels Dec 18, 2018
@miri64 miri64 force-pushed the gnrc_ipv6_ext_rh/enh/icmpv6_error_msg branch from fd1506a to be5dd81 Compare December 18, 2018 18:46
@miri64
Copy link
Member Author

miri64 commented Dec 18, 2018

(and squashed)

No longer waiting on other PRs.

@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
@miri64
Copy link
Member Author

miri64 commented Jan 2, 2019

Would be great if we can finish this project for the release, so we can include #10382 and #10392 into the release tests.

@miri64
Copy link
Member Author

miri64 commented Jan 2, 2019

@aabadie @jia200x are you maybe able to review, or even @bergzand?

@jia200x
Copy link
Member

jia200x commented Jan 2, 2019

I'm already on it ;)

@jia200x jia200x added 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 labels Jan 2, 2019
@jia200x
Copy link
Member

jia200x commented Jan 2, 2019

code design makes sense

@jia200x
Copy link
Member

jia200x commented Jan 2, 2019

vera++ doesn't report errors nor warnings

@jia200x jia200x added the Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines label Jan 2, 2019
@jia200x
Copy link
Member

jia200x commented Jan 2, 2019

tests make sense and pass. I will do more manual tests tomorrow in the morning.

@jia200x
Copy link
Member

jia200x commented Jan 2, 2019

documentation looks ok, and displayed as expected in Doxygen

@jia200x jia200x added the Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines label Jan 2, 2019
@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 Jan 2, 2019
@jia200x
Copy link
Member

jia200x commented Jan 3, 2019

ok, I receive ICMPv6 Parameter Problem and Time Exceed (hop limit) after sending malformed packets. As expected

@jia200x jia200x added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jan 3, 2019
jia200x
jia200x approved these changes Jan 3, 2019
View reviewed changes
Copy link
Member

@jia200x jia200x left a comment

Choose a reason for hiding this comment

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

ACK

@jia200x
Copy link
Member

jia200x commented Jan 3, 2019

& GO. Thanks for your contribution!

@jia200x jia200x merged commit f14d46d into RIOT-OS:master Jan 3, 2019
@miri64 miri64 deleted the gnrc_ipv6_ext_rh/enh/icmpv6_error_msg branch January 3, 2019 11:13
@miri64
Copy link
Member Author

miri64 commented Jan 3, 2019

Thanks for the review!

@aabadie aabadie added this to the Release 2019.01 milestone Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jia200x jia200x jia200x approved these changes

@bergzand bergzand Awaiting requested review from bergzand

Assignees
No one assigned
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: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2019.01
Development

Successfully merging this pull request may close these issues.
3 participants
@miri64 @jia200x @aabadie