Skip to content
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.

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

gnrc_ipv6: iface might be from input on next hop determination #3935

Merged
merged 1 commit into from
Sep 22, 2015

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Sep 22, 2015

@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking NSTF CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 22, 2015
@miri64 miri64 added this to the Release 2015.09 milestone Sep 22, 2015
@miri64 miri64 force-pushed the gnrc_ipv6/fix/iface-may-be-from-input branch from aefb74f to 6fef744 Compare September 22, 2015 17:03
@miri64
Copy link
Member Author

miri64 commented Sep 22, 2015

Actually… we don't need #3931 at all… The if-return in https://github.com/RIOT-OS/RIOT/pull/3935/files#diff-74976d45a42eaa063e3561b5fd36f615R536-538 assures already, that the function is exited before the next hop determination of standard IPv6 is called....

@miri64
Copy link
Member Author

miri64 commented Sep 22, 2015

Some fine reviewing I did there 😞

@OlegHahm
Copy link
Member

The if-return is only for the error-case. #3931 is required - I actually ran into problems with this.

@miri64
Copy link
Member Author

miri64 commented Sep 22, 2015

Okay let's write the code from #3931 with the assumption that all #ifdefs are true:

    (void)pkt;
    iface = gnrc_sixlowpan_nd_next_hop_l2addr(l2addr, l2addr_len, iface, dst);
    if (iface <= KERNEL_PID_UNDEF) {
        return iface;
    }
    if (iface <= KERNEL_PID_UNDEF) {
        iface = gnrc_ndp_node_next_hop_l2addr(l2addr, l2addr_len, iface, dst, pkt);
    }

Do you see what I'm getting at? Two times the same check, the first results in returning. So how could iface be <= KERNEL_PID_UNDEF in the code introduced in #3931?

@miri64
Copy link
Member Author

miri64 commented Sep 22, 2015

(except if it was given on input).

@OlegHahm
Copy link
Member

It is about the case where iface > KERNEL_PID_UNDEF.

@OlegHahm
Copy link
Member

We're setting the interface twice in this case and apparently the later setting overwrites the first one in a wrong manner.

@miri64
Copy link
Member Author

miri64 commented Sep 22, 2015

Ahhh… now I see what you mean… okay I revert this PR to its original state.

@miri64 miri64 force-pushed the gnrc_ipv6/fix/iface-may-be-from-input branch from 6fef744 to aefb74f Compare September 22, 2015 17:20
@miri64
Copy link
Member Author

miri64 commented Sep 22, 2015

Done

@@ -530,26 +530,29 @@ static inline kernel_pid_t _next_hop_l2addr(uint8_t *l2addr, uint8_t *l2addr_len
kernel_pid_t iface, ipv6_addr_t *dst,
gnrc_pktsnip_t *pkt)
{
kernel_pid_t found_iface;
Copy link
Member

Choose a reason for hiding this comment

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

If you initialize this with KERNEL_PID_UNDEF, you can save the #else part in line 540/541.

Copy link
Member Author

Choose a reason for hiding this comment

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

See fixup (and I did this because otherwise cppcheck would have been complaining)

@OlegHahm
Copy link
Member

Please squash

@miri64 miri64 force-pushed the gnrc_ipv6/fix/iface-may-be-from-input branch from 04878de to 5d75016 Compare September 22, 2015 18:43
@miri64
Copy link
Member Author

miri64 commented Sep 22, 2015

Squashed

miri64 added a commit that referenced this pull request Sep 22, 2015
…-from-input

gnrc_ipv6: iface might be from input on next hop determination
@miri64 miri64 merged commit 549a356 into RIOT-OS:master Sep 22, 2015
@miri64 miri64 deleted the gnrc_ipv6/fix/iface-may-be-from-input branch September 22, 2015 19:25
@OlegHahm
Copy link
Member

Technically there was no ACK, but should be fine anyway.

@miri64
Copy link
Member Author

miri64 commented Sep 22, 2015

Sorry 😞

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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants