gnrc_icmpv6_error: initial import by miri64 · Pull Request #3184 · RIOT-OS/RIOT

miri64 · 2015-06-10T02:04:26Z

Provides capabilities to build ICMPv6 error messages.

miri64 · 2015-08-03T09:57:25Z

Removed some code duplication.

OlegHahm · 2015-08-10T15:17:11Z

sys/net/network_layer/ng_icmpv6/error/ng_icmpv6_error.c

I guess a macro would be fine for this.

miri64 · 2015-08-11T11:47:05Z

addressed comments.

miri64 · 2015-08-11T11:48:16Z

Rebased

miri64 · 2015-08-18T22:35:20Z

Rebased and adapted to current master

OlegHahm · 2015-08-22T10:27:51Z

sys/net/gnrc/network_layer/icmpv6/error/gnrc_icmpv6_error.c

What's the purpose of these wrapper functions? Don't seem to shorten anything.

They are the API functions.

All other ICMPv6 messages have their own build functions, too. So it would be inconsistent to have only two (these three in one and the param_prob one) for the errors. Especially since the 4th parameter of _icmpv6_error_build() is for the most cases just used to set the reserved field.

All other ICMPv6 messages have their own build functions, too

With #3693 not any more. ;)

I think providing _icmpv6_error_build() as an API function is sufficient.

The case of gnrc_icmpv6_echo_build() is a totally different one as this one. A generic gnrc_icmpv6_error_build() would expose reserved fields (which have to be 0) to the user.

Also how would you understandably document the fourth parameter?

Sets the x-th offseted word in the packet. Needs to be 0 for ICMPV6_DST_UNR and ICMPV6_TIME_EXC and the MTU of the interface for ICMPV6_PKT_TOO_BIG.

?

Which "user" needs to send ICMPv6 echo packets?

Me e.g. (and these are echo packages). Let's discuss this on Monday f2f.

any result of this discussion?

#3693 was merged, but I still argue that it doesn't make sense here. The different message types have different default values (some need code to be 0, some need the "value" field [which is not existent in any of the packet types] to be 0 [when it is the unused-field]) and param_prob even expects a pointer inside orig_pkt making it the odd one out and an API that would be like @OlegHahm proposed inconsistent.

miri64 · 2015-08-22T12:22:57Z

Rebased to #3691 and addressed comments

cgundogan · 2015-10-27T14:51:21Z

needs a rebase to master, since #3691 is no more.

OlegHahm · 2015-11-24T18:35:39Z

Is this PR still waiting on anything?

miri64 · 2015-11-24T20:47:32Z

Nope, rebased to current master (the commit in master was still in here), squashed and adapted commit message

OlegHahm · 2015-11-24T21:01:21Z

Untested ACK.

Yonezawa-T2 · 2015-12-24T10:23:48Z

sys/net/gnrc/network_layer/icmpv6/error/gnrc_icmpv6_error.c

Can GNRC_IPV6_NETIF_DEFAULT_MTU be greater than IPV6_MIN_MTU? RFC requires it must be less than IPV6_MIN_MTU https://tools.ietf.org/html/rfc4443#section-2.4

The RFC refers to the minimum MTU as a minimum, so a packet MUST NOT be smaller than it. It is pretty common to have a bigger MTU: The MTU for Ethernet is typically 1500 (which is exactly the maximum packet size of Ethernet)

It says "without making the error message packet exceed the minimum IPv6 MTU [IPv6]." Doesn't it means we should not exceed 1280 bytes?

Sorry, my intent is that "it" in "RFC requires it must be less than ..." refers the return value of the _fit function. Very confusing sentence.

Ah yes, the error message's size must not be greater than the minimum MTU. This implementation is older than the introduction of IPV6_MIN_MTU so it needs to be adapted, yes.

miri64 · 2016-01-23T12:54:14Z

Generalized and centralized.

miri64 · 2016-01-23T13:02:30Z

I meant rebased and adapted to current master…

OlegHahm · 2016-01-26T17:23:52Z

Code was already acked, doesn't seem to break anything, and also should have no affects for other stuff. ACK and go!

gnrc_icmpv6_error: initial import

miri64 added Type: new feature The issue requests / The PR implemements a new feature for RIOT NSTF labels Jun 10, 2015

miri64 assigned OlegHahm Jun 10, 2015

miri64 added this to the Network Stack Task Force milestone Jun 10, 2015

miri64 force-pushed the ng_icmpv6_error/feat/init branch from b153ed7 to fbbf7cc Compare June 10, 2015 02:05

miri64 mentioned this pull request Aug 3, 2015

Make use of doxygen todo #3536

Merged

miri64 force-pushed the ng_icmpv6_error/feat/init branch from 95bb856 to e900705 Compare August 3, 2015 09:57

miri64 mentioned this pull request Aug 3, 2015

gnrc_icmpv6_error: Initial import of IPv6 error convenience functions #3544

Merged

OlegHahm reviewed Aug 10, 2015
View reviewed changes

sys/net/network_layer/ng_icmpv6/error/ng_icmpv6_error.c Outdated

Copy link
Copy Markdown

Member

OlegHahm Aug 10, 2015

Choose a reason for hiding this comment

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

I guess a macro would be fine for this.

miri64 force-pushed the ng_icmpv6_error/feat/init branch from 6d77a49 to 10ac802 Compare August 11, 2015 11:48

miri64 force-pushed the ng_icmpv6_error/feat/init branch from 10ac802 to 44e466d Compare August 18, 2015 22:35

miri64 changed the title ~~ng_icmpv6_error: initial import~~ gnrc_icmpv6_error: initial import Aug 18, 2015

OlegHahm reviewed Aug 22, 2015
View reviewed changes

miri64 force-pushed the ng_icmpv6_error/feat/init branch from c7e0096 to 32edf53 Compare August 22, 2015 12:20

miri64 added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable State: waiting for other PR State: The PR requires another PR to be merged first labels Aug 22, 2015

miri64 mentioned this pull request Aug 31, 2015

Network stack refactoring: TODO list #2278

Closed

36 tasks

miri64 modified the milestones: Release NEXT MAJOR, Network Stack Task Force Sep 2, 2015

miri64 force-pushed the ng_icmpv6_error/feat/init branch from 32edf53 to fb97bb6 Compare November 24, 2015 20:46

miri64 removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable State: waiting for other PR State: The PR requires another PR to be merged first labels Nov 24, 2015

OlegHahm modified the milestones: Release 2015.12, Release 2016.03 Dec 8, 2015

miri64 mentioned this pull request Dec 24, 2015

gnrc_ipv6: send ICMPv6 "Packet Too Big Message" if packet is too big #4546

Closed

Yonezawa-T2 reviewed Dec 24, 2015
View reviewed changes

miri64 force-pushed the ng_icmpv6_error/feat/init branch from fb97bb6 to d1cdee2 Compare January 23, 2016 12:54

miri64 added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Jan 23, 2016

miri64 force-pushed the ng_icmpv6_error/feat/init branch from d1cdee2 to 80198f1 Compare January 23, 2016 13:01

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 23, 2016

miri64 force-pushed the ng_icmpv6_error/feat/init branch 2 times, most recently from c1e41eb to 27dc1b4 Compare January 23, 2016 15:02

gnrc_icmpv6_error: initial import

70c3d29

miri64 force-pushed the ng_icmpv6_error/feat/init branch from 27dc1b4 to 70c3d29 Compare January 23, 2016 15:04

OlegHahm added a commit that referenced this pull request Jan 26, 2016

Merge pull request #3184 from authmillenon/ng_icmpv6_error/feat/init

3d494c8

gnrc_icmpv6_error: initial import

OlegHahm merged commit 3d494c8 into RIOT-OS:master Jan 26, 2016

miri64 deleted the ng_icmpv6_error/feat/init branch January 26, 2016 17:27

miri64 added the Area: network Area: Networking label Sep 30, 2018

Conversation

miri64 commented Jun 10, 2015

Uh oh!

miri64 commented Aug 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Aug 11, 2015

Uh oh!

miri64 commented Aug 11, 2015

Uh oh!

miri64 commented Aug 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Aug 22, 2015

Uh oh!

cgundogan commented Oct 27, 2015

Uh oh!

OlegHahm commented Nov 24, 2015

Uh oh!

miri64 commented Nov 24, 2015

Uh oh!

OlegHahm commented Nov 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jan 23, 2016

Uh oh!

miri64 commented Jan 23, 2016

Uh oh!

OlegHahm commented Jan 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants