Skip to content

gnrc_ipv6: accept packets for global dst at all interfaces#4738

Merged
OlegHahm merged 1 commit intoRIOT-OS:masterfrom
miri64:gnrc_ipv6/fix/accept-dst-for-all-if
Mar 30, 2016
Merged

gnrc_ipv6: accept packets for global dst at all interfaces#4738
OlegHahm merged 1 commit intoRIOT-OS:masterfrom
miri64:gnrc_ipv6/fix/accept-dst-for-all-if

Conversation

@miri64
Copy link
Copy Markdown
Member

@miri64 miri64 commented Feb 4, 2016

Alternative to #4456.

@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking NSTF labels Feb 4, 2016
@miri64 miri64 added this to the Release 2016.03 milestone Feb 4, 2016
This was referenced Feb 4, 2016
Closed
Closed
@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Feb 22, 2016

How did you test this?

I tried this using the same setup as in #4456.
In that setup, the icmp echo is correctly received, but the reply is sent over the wrong interface.

This PR only touches forwarding (as only _pkt_not_for_me is changed).

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Feb 22, 2016

This PR only touches forwarding (as only _pkt_not_for_me is changed).

Sorry, not correct...

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Feb 22, 2016

Ok, I tested a wrong setup (using link-local addresses and a fib default route).
With the same setup as in in #4456 (this time for real), I can ping the 6lowpan address of 6lbr.

@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 22, 2016
Yonezawa-T2
Yonezawa-T2 reviewed Feb 23, 2016
View reviewed changes
sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c Outdated
else if ((!ipv6_addr_is_link_local(&hdr->dst)) ||
(*iface == KERNEL_PID_UNDEF)) {
kernel_pid_t if_pid = gnrc_ipv6_netif_find_by_addr(NULL, &hdr->dst);
if (*iface != KERNEL_PID_UNDEF) {
Copy link
Copy Markdown
Contributor

@Yonezawa-T2 Yonezawa-T2 Feb 23, 2016

Choose a reason for hiding this comment

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

*iface == KERNEL_PID_UNDEF?

Copy link
Copy Markdown
Member Author

@miri64 miri64 Feb 23, 2016

Choose a reason for hiding this comment

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

Who needs logic :P (yes)

Copy link
Copy Markdown
Member Author

@miri64 miri64 Feb 23, 2016

Choose a reason for hiding this comment

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

Wait no, it should be changed in any case. :-)

Copy link
Copy Markdown
Member Author

@miri64 miri64 Feb 23, 2016

Choose a reason for hiding this comment

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

okay, let's think about this ... to fix the @kaspar030's issue *iface must not be changed at all (in that case *iface shouldn't be KERNEL_PID_UNDEF). If no interface is given (*iface == KERNEL_PID_UNDEF) *iface should be overwritten. So your original comment was correct.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Feb 23, 2016

Addressed @Yonezawa-T2's comment.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Feb 23, 2016

@kaspar030 can you test again? Only read your corrections afterwards..... Now I'm not sure this is actually a fix. Will revert as soon as you say this isn't working anymore.

@OlegHahm OlegHahm assigned kaspar030 and unassigned OlegHahm Feb 23, 2016
@OlegHahm OlegHahm 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 Feb 27, 2016
@botmayank botmayank mentioned this pull request Mar 8, 2016
Merged
@cgundogan
Copy link
Copy Markdown
Member

cgundogan commented Mar 29, 2016

looks good. ACK

@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 Mar 29, 2016
@OlegHahm OlegHahm added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 29, 2016
@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Mar 29, 2016

Needs squashing.

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Mar 30, 2016

I just re-checked that this fixes the bug where RIOT was not answering to all of it's addresses.

@miri64 miri64 force-pushed the gnrc_ipv6/fix/accept-dst-for-all-if branch from 1471c21 to 5532f92 Compare March 30, 2016 12:54
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Mar 30, 2016

Squashed.

@OlegHahm OlegHahm added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Mar 30, 2016
@OlegHahm OlegHahm merged commit 230e105 into RIOT-OS:master Mar 30, 2016
@miri64 miri64 deleted the gnrc_ipv6/fix/accept-dst-for-all-if branch March 30, 2016 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@kaspar030 kaspar030

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 Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2016.04

Development

Successfully merging this pull request may close these issues.

5 participants

@miri64 @kaspar030 @cgundogan @OlegHahm @Yonezawa-T2