gnrc_ipv6_ext: move ipv6_ext_rh (partly) to GNRC by miri64 · Pull Request #10231 · RIOT-OS/RIOT

miri64 · 2018-10-23T18:03:42Z

Contribution description

This moves all implementation specific parts of the ipv6_ext_rh module to gnrc_ipv6_ext_rh.

It also removes a module that isn't used after this change and doesn't contain any code (but isn't a pseudo-module for some reason).

Testing procedure

Try to compile gnrc_networking with USEMODULE = gnrc_rpl_srh. It should still work.

Also (at least when run without DEBUG) tests/gnrc_ipv6_ext should still work without crashing (I will rework them once the integration process progressed).

Issues/PRs references

Addresses parts of #10133

… HEAD

…o HEAD

… HEAD

miri64 · 2018-10-24T19:02:40Z

@bergzand could you maybe have a look (if doesn't intervene with the current release cycle)? @cgundogan is currently on vacation and this PR is giving me a really hard time, when I try to rebase #10238 on top of it and #10234 (which I do for testing).

miri64 · 2018-10-24T19:08:32Z

Rebased and squashed current state, so tracking during the review is easier.

miri64 · 2018-10-24T19:22:21Z

I also made my train of thought a little clearer by splitting out one commit (the second one, 692eaa0 while I was writing this) of another.

… HEAD

bergzand · 2018-10-25T08:08:34Z

The motivation for this change is to have these functions inside GNRC so that the structs/arguments can be kept GNRC-specific?

bergzand

First review iteration here.

sys/include/net/gnrc/rpl/srh.h

sys/include/net/ipv6/ext/rh.h

sys/net/gnrc/network_layer/ipv6/ext/gnrc_ipv6_ext.c

miri64 · 2018-10-25T08:16:55Z

The motivation for this change is to have these functions inside GNRC so that the structs/arguments can be kept GNRC-specific?

Yes, and also that ipv6_ext_rh_process() should have never been outside of GNRC (the non-GNRC modules are just thought to be used for header parsing), as it straight jumps back into GNRC (for RPL SRH processing). Also there is a "GNRC-version" of this function which just adds another indirection that isn't necessary (see #10238).

bergzand

A few more nits.

bergzand · 2018-10-25T08:26:15Z

sys/include/net/gnrc/ipv6/ext/rh.h

+    GNRC_IPV6_EXT_RH_OK = 0,
+    /**
+     * @brief   The routing header was successfully processed and the packet
+     *          was forwarded or should be forwarded by the central RH handler.


Something that "was forwarded" could have this node as destionation right? Does the GNRC_IPV6_EXT_RH_OK have priority over this result?

Can you rephrase this? I'm not sure I understand.

Ah I get it now

Maybe I'm interpreting the "was forwarded" here not correct. To me it sounds as if a frame was forwarded by another node to this node. Where at that point both the conditions from this documentation and the option currently renamed to GNRC_IPV6_EXT_RH_AT_DST match the frame.

Because it was both forwarded to this node and at the destination.

Maybe the documentation update clarifies it better.

bergzand · 2018-10-25T08:28:43Z

sys/include/net/gnrc/ipv6/ext/rh.h

+     * @brief   The routing header was successfully processed and this node
+     *          is the destination
+     */
+    GNRC_IPV6_EXT_RH_OK = 0,


Does it make sense to change the name here to something that indicates that the current node is the destination such as GNRC_IPV6_EXT_RH_DEST (Feel free to change to something that makes more sense to you, my lack of coffee prevents me from thinking of a sensible name)

GNRC_IPV6_EXT_RH_AT_DST?

(since this enum values are providing the most headache when rebasing, this will be fun to rename ;-D, but I think you are right regarding the naming)

miri64 · 2018-10-25T08:55:16Z

I changed the order of the enum, as it makes more sense when = 0 is the error and > 0 one of the two success cases.

miri64 · 2018-10-25T08:56:48Z

I just noticed, that I did a terrible mistake with my fixup commits. May I squash?

bergzand · 2018-10-25T08:57:19Z

May I squash?

Ack

miri64 · 2018-10-25T09:07:45Z

Squashed

Those return values are internal anyway, and they have the same intent as the one provided by `gnrc_ipv6_ext_rh`.

miri64 · 2018-10-25T09:13:22Z

I put the rename of _OK to _AT_DST to its own commit for clarity in case someone reviews the git history later ;-).

bergzand

Tested, ACK

miri64 · 2018-10-25T09:22:32Z

@jia200x since this is an API change (and thus high-impact IMHO) I would merge this right after feature freeze (i.e. when you forked the release branch). Are you alright with that?

jia200x · 2018-10-25T09:23:36Z

@jia200x since this is an API change (and thus high-impact IMHO) I would merge this right after feature freeze (i.e. when you forked the release branch). Are you alright with that?

Yes, I'm alright ;)

miri64 · 2018-10-25T12:03:12Z

Adapted the unittests for the value name changes as well.

miri64 · 2018-10-25T17:15:19Z

Release is feature frozen, so I'm merging this now.

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 23, 2018

miri64 requested a review from cgundogan October 23, 2018 18:03

miri64 mentioned this pull request Oct 23, 2018

gnrc_ipv6_ext: needs heavy refactoring #10133

Closed

5 tasks

miri64 force-pushed the gnrc_ipv6_ext/enh/move-rh-to-gnrc branch 2 times, most recently from 56fdbf7 to 97784fe Compare October 23, 2018 18:08

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 23, 2018

Merge branch 'gnrc_ipv6_ext/enh/move-rh-to-gnrc' (RIOT-OS#10231) into…

01c5dbd

… HEAD

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 23, 2018

Merge branch 'gnrc_ipv6_ext/enh/move-rh-to-gnrc' (RIOT-OS#10231) int…

9516bd5

…o HEAD

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 24, 2018

Merge branch 'gnrc_ipv6_ext/enh/move-rh-to-gnrc' (RIOT-OS#10231) into…

71e163c

… HEAD

miri64 mentioned this pull request Oct 24, 2018

gnrc_ipv6_ext: merge _handle_rh and gnrc_ipv6_ext_rh_process #10238

Merged

miri64 force-pushed the gnrc_ipv6_ext/enh/move-rh-to-gnrc branch from f251de6 to 5683c4e Compare October 24, 2018 19:08

miri64 force-pushed the gnrc_ipv6_ext/enh/move-rh-to-gnrc branch from 5683c4e to 90607e3 Compare October 24, 2018 19:22

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 24, 2018

Merge branch 'gnrc_ipv6_ext/enh/move-rh-to-gnrc' (RIOT-OS#10231) into…

f527e62

… HEAD

bergzand self-assigned this Oct 25, 2018

bergzand self-requested a review October 25, 2018 08:06

bergzand requested changes Oct 25, 2018

View reviewed changes

sys/include/net/gnrc/rpl/srh.h Show resolved Hide resolved

sys/include/net/ipv6/ext/rh.h Show resolved Hide resolved

sys/net/gnrc/network_layer/ipv6/ext/gnrc_ipv6_ext.c Show resolved Hide resolved

bergzand requested changes Oct 25, 2018

View reviewed changes

miri64 force-pushed the gnrc_ipv6_ext/enh/move-rh-to-gnrc branch from eab20ad to e9fa72e Compare October 25, 2018 09:06

miri64 and others added 2 commits October 25, 2018 11:10

gnrc_ipv6_ext: move ipv6_ext_rh (partly) to GNRC

c54ba49

gnrc_ipv6_ext_rh: rename GNRC_IPV6_EXT_RH_OK for clarity

84a62da

miri64 added 2 commits October 25, 2018 11:12

gnrc_ipv6_ext: remove unnecessary return-value indirection

6a9adf2

Those return values are internal anyway, and they have the same intent as the one provided by `gnrc_ipv6_ext_rh`.

ipv6_ext: remove empty, unused module

ad183db

miri64 force-pushed the gnrc_ipv6_ext/enh/move-rh-to-gnrc branch from e9fa72e to ad183db Compare October 25, 2018 09:12

bergzand added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 25, 2018

bergzand approved these changes Oct 25, 2018

View reviewed changes

tests/unittests: adapt for value name changes

390ddfb

miri64 merged commit 466bc34 into RIOT-OS:master Oct 25, 2018

miri64 deleted the gnrc_ipv6_ext/enh/move-rh-to-gnrc branch October 25, 2018 20:01

Conversation

miri64 commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 commented Oct 24, 2018

Uh oh!

miri64 commented Oct 24, 2018

Uh oh!

miri64 commented Oct 24, 2018

Uh oh!

bergzand commented Oct 25, 2018

Uh oh!

bergzand left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miri64 commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bergzand left a comment

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!

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 Oct 25, 2018

Uh oh!

miri64 commented Oct 25, 2018

Uh oh!

bergzand commented Oct 25, 2018

Uh oh!

miri64 commented Oct 25, 2018

Uh oh!

miri64 commented Oct 25, 2018

Uh oh!

bergzand left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Oct 25, 2018

Uh oh!

jia200x commented Oct 25, 2018

Uh oh!

miri64 commented Oct 25, 2018

Uh oh!

miri64 commented Oct 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

miri64 commented Oct 23, 2018 •

edited

Loading

miri64 commented Oct 25, 2018 •

edited

Loading