gnrc_ndp_host: Initial import of host behavior of router discovery #3746

miri64 · 2015-08-31T13:26:03Z

This imports host behavior for NDP router discovery. This encompasses:

building and sending of (semi-periodic) router solicitations (including all available options specified in RFC 4861)
handling of router advertisements and their options

Depends on:

~~gnrc_ndp: add support for address-less link-layers #3628: address-less link-layer handling for NDP~~
~~gnrc_ipv6_netif: prepare for router discovery #3745: gnrc_ipv6_netif adaptations for router discovery~~ (merged)

miri64 · 2015-08-31T20:05:46Z

Rebased to current dependencies

OlegHahm · 2015-08-31T21:57:08Z

sys/include/net/gnrc/ipv6/netif.h

@@ -264,6 +338,7 @@ void gnrc_ipv6_netif_remove(kernel_pid_t pid);
 */
 gnrc_ipv6_netif_t *gnrc_ipv6_netif_get(kernel_pid_t pid);

+#if (defined(MODULE_GNRC_NDP_ROUTER) || defined(MODULE_GNRC_SIXLOWPAN_ND_ROUTER))


What's the purpose of this ifdef?

This was introduced in #3745 and proposed by @cgundogan (#3745 (comment)). I liked the idea because it prevents that the function is called 'accidentally' ;-)

But to answer your question in a direct manner: To generate NOP macros in the else case. (You will see this technique quite a bit throughout the NDP-expansions)

miri64 · 2015-08-31T22:53:01Z

Rebased to current #3628 to incorporate miri64#17.

miri64 · 2015-09-01T17:52:37Z

Rebased to current master and dependencies.

miri64 · 2015-09-02T10:41:36Z

sys/net/gnrc/network_layer/ndp/gnrc_ndp.c

+        /* calculate new random reachable time */
+        uint32_t reach_time = genrand_uint32_range(GNRC_NDP_MIN_RAND, GNRC_NDP_MAX_RAND);
+        if_entry->reach_time_base = byteorder_ntohl(rtr_adv->reach_time);
+        reach_time = (reach_time * if_entry->reach_time_base) / 10;


The 10 comes from the fact, that GNRC_NDP_MIN_RAND and GNRC_NDP_MAX_RAND are bigger by a factor of 10 than in the RFC (because they are fractions). Don't know how to express that, other than already documented here: https://github.com/RIOT-OS/RIOT/blob/master/sys/include/net/gnrc/ipv6/netif.h#L296

Actually, I understand neither this explanation nor the relation to the documentation of reach_time_base`.

reach_time is calculated by taking gnrc_ipv6_netif_t::reach_time_base (set by the advertisement) and multiply it by a random value between 0.5 and 1.5 (https://tools.ietf.org/html/rfc4861#section-10). To avoid floating point numbers (and also have some variation) I used 5 and 15 as values for GNRC_NDP_MIN_RAND and GNRC_NDP_MAX_RAND. To get back to the original value I devided by ten (as I did on interface initialization)

Ok, so, how about a comment here saying something like "to avoid floating point number computations, the boundaries for the random values are multiplied by 10"?

"to avoid floating point number computations, the boundaries for the random values are multiplied by 10"

"in order to avoid division, we divide." :)

"in order to avoid division, we divide." :)

I did not say to avoid division, I said to avoid going to floating point computations ;-) I basically introduce a low-profile fixed-point number calculation here, if you want to call it that ;-P

Just add a comment here and all is fine.

miri64 · 2015-09-02T10:43:49Z

Addressed comments in previous commits.

OlegHahm · 2015-09-02T12:06:55Z

Please rebase

miri64 · 2015-09-02T12:11:19Z

Rebased to current master

OlegHahm · 2015-09-02T12:56:02Z

sys/net/gnrc/network_layer/ndp/gnrc_ndp.c

+    int sicmpv6_size = (int)icmpv6_size, l2src_len = 0;
+    uint16_t opt_offset = 0;
+    assert(if_entry != NULL);
+    if (!ipv6_addr_is_link_local(&ipv6->src) ||


Styling question, but for readability I prefer a newline between variable initializations and actual start of the functionality.

I agree. Don't know, why I did it that way.

OlegHahm · 2015-09-02T14:59:37Z

sys/net/gnrc/network_layer/ndp/gnrc_ndp.c

+    /* set flags from message */
+    if_entry->flags &= ~GNRC_IPV6_NETIF_FLAGS_RTR_ADV_MASK;
+    if_entry->flags |= ((rtr_adv->flags << GNRC_IPV6_NETIF_FLAGS_RTR_ADV_POS) &
+                        (GNRC_IPV6_NETIF_FLAGS_RTR_ADV_MASK));


nitpicking, but I think you overdid with parens here. ;)

Who are you and where is @OlegHahm?

OlegHahm · 2015-09-02T15:22:49Z

sys/net/gnrc/network_layer/ndp/internal/gnrc_ndp_internal.c

+                                     ndp_opt_pi_t *pi_opt)
+{
+    ipv6_addr_t *prefix;
+    gnrc_ipv6_netif_addr_t *a;


I would prefer addr or something more speaking.

OlegHahm · 2015-09-02T15:25:42Z

apart from the minor comments: code is okay, test didn't break anything. ACK after addressing and squashing if Travis agrees, too.

miri64 · 2015-09-02T15:31:16Z

Addressed and squashed immediately. One minor thing I changed: I put the calculation and setting of reachable time related gnrc_ipv6_netif_t members into their own inline function and used that function on interface initialization as well.

gnrc_ndp_host: Initial import of host behavior of router discovery

miri64 · 2015-09-02T16:59:36Z

Thanks for the review! :-)

miri64 assigned OlegHahm Aug 31, 2015

miri64 added this to the Release 2015.08 milestone Aug 31, 2015

miri64 mentioned this pull request Aug 31, 2015

gnrc_ndp_router: Initial import of router behavior of router discovery #3049

Merged

miri64 force-pushed the gnrc_ndp_host/feat/initial branch from 128fbe4 to 67c7ca4 Compare August 31, 2015 13:34

miri64 force-pushed the gnrc_ndp_host/feat/initial branch from 67c7ca4 to e9d77f9 Compare August 31, 2015 20:05

OlegHahm reviewed Aug 31, 2015
View reviewed changes

miri64 force-pushed the gnrc_ndp_host/feat/initial branch from e9d77f9 to d89da6d Compare August 31, 2015 22:49

miri64 force-pushed the gnrc_ndp_host/feat/initial branch 2 times, most recently from d1c8aa2 to 8878f4d Compare September 1, 2015 17:52

miri64 reviewed Sep 2, 2015
View reviewed changes

OlegHahm removed the State: waiting for other PR State: The PR requires another PR to be merged first label Sep 2, 2015

miri64 force-pushed the gnrc_ndp_host/feat/initial branch from 14df47e to 1a84e42 Compare September 2, 2015 12:11

miri64 force-pushed the gnrc_ndp_host/feat/initial branch from 1a84e42 to f76d89d Compare September 2, 2015 12:16

OlegHahm added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 2, 2015

OlegHahm reviewed Sep 2, 2015
View reviewed changes

OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Sep 2, 2015

OlegHahm reviewed Sep 2, 2015
View reviewed changes

miri64 added 2 commits September 2, 2015 17:31

gnrc_ndp_host: initial import

b033ff5

gnrc: adapt for gnrc_ndp_host

c367477

miri64 force-pushed the gnrc_ndp_host/feat/initial branch from 3fcf32f to c367477 Compare September 2, 2015 15:31

miri64 removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Sep 2, 2015

OlegHahm added a commit that referenced this pull request Sep 2, 2015

Merge pull request #3746 from authmillenon/gnrc_ndp_host/feat/initial

b625d72

gnrc_ndp_host: Initial import of host behavior of router discovery

OlegHahm merged commit b625d72 into RIOT-OS:master Sep 2, 2015

miri64 deleted the gnrc_ndp_host/feat/initial branch September 2, 2015 16:54

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gnrc_ndp_host: Initial import of host behavior of router discovery #3746

gnrc_ndp_host: Initial import of host behavior of router discovery #3746

miri64 commented Aug 31, 2015

miri64 commented Aug 31, 2015

OlegHahm Aug 31, 2015

miri64 Aug 31, 2015

miri64 Aug 31, 2015

miri64 commented Aug 31, 2015

miri64 commented Sep 1, 2015

miri64 Sep 2, 2015

OlegHahm Sep 2, 2015

miri64 Sep 2, 2015

OlegHahm Sep 2, 2015

kaspar030 Sep 2, 2015 via email

miri64 Sep 2, 2015

OlegHahm Sep 2, 2015

miri64 commented Sep 2, 2015

OlegHahm commented Sep 2, 2015

miri64 commented Sep 2, 2015

OlegHahm Sep 2, 2015

miri64 Sep 2, 2015

OlegHahm Sep 2, 2015

miri64 Sep 2, 2015

OlegHahm Sep 2, 2015

OlegHahm Sep 2, 2015

OlegHahm commented Sep 2, 2015

miri64 commented Sep 2, 2015

miri64 commented Sep 2, 2015

gnrc_ndp_host: Initial import of host behavior of router discovery #3746

gnrc_ndp_host: Initial import of host behavior of router discovery #3746

Conversation

miri64 commented Aug 31, 2015

miri64 commented Aug 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Aug 31, 2015

miri64 commented Sep 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspar030 Sep 2, 2015 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Sep 2, 2015

OlegHahm commented Sep 2, 2015

miri64 commented Sep 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlegHahm commented Sep 2, 2015

miri64 commented Sep 2, 2015

miri64 commented Sep 2, 2015