gnrc_ipv6_nib: provide internal implementation for off-link entries #7212

miri64 · 2017-06-20T13:28:45Z

Implementation off the off-link component of the NIB back-end.

I needed to move some stuff around, to keep consistency, but other than that the change should be straight forward.

This PR is part of the network layer remodelling effort:

smlng · 2017-06-21T14:49:42Z

sys/include/net/gnrc/ipv6/nib/conf.h

+/**
+ * @brief   Number of off-link entries in NIB
+ *
+ * @note    **This number has direct influence on the maximum number of


instead of enclosing this with ** ... ** you could rather use @attention instead of @note

Tried to keep the style of the original ones.

I understand - and docu styling might be separate more general issue anyway 😉

Will fix in a different PR ;-)

@smlng see #7218

…HEAD

…HEAD

cgundogan

first round ..

cgundogan · 2017-08-03T13:15:36Z

sys/include/net/gnrc/ipv6/nib/conf.h

+/**
+ * @brief   Number of off-link entries in NIB
+ *
+ * @attention   This number has direct influence on the maximum number of


just to clarify: how exactly does GNRC_IPV6_NIB_OFFL_NUMOF influences the max. number? I would assume it is the max. number, or is there another number that is added to that?

cgundogan · 2017-08-03T13:18:36Z

sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.c

    DEBUG("nib: Allocating on-link node entry (addr = %s, iface = %u)\n",
          ipv6_addr_to_str(addr_str, addr, sizeof(addr_str)), iface);
    for (unsigned i = 0; i < GNRC_IPV6_NIB_NUMOF; i++) {
        _nib_onl_entry_t *tmp = &_nodes[i];

        if ((_nib_onl_get_if(tmp) == iface) &&
-            (ipv6_addr_equal(addr, &tmp->ipv6))) {
+            ((addr == NULL) || (ipv6_addr_equal(addr, &tmp->ipv6)))) {


why do you need this check for addr == NULL here? There's a @pre in the doxygen of this function, that addr != NULL.

you probably need to remove the doxygen @pre then.

The @pre is wrong. For prefix list entries the next-hop address may be NULL.

Found an error here, while debugging NDP-RD. Will fix.

cgundogan · 2017-08-03T13:20:37Z

sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.c

@@ -62,14 +64,13 @@ _nib_onl_entry_t *_nib_onl_alloc(const ipv6_addr_t *addr, unsigned iface)
 {
    _nib_onl_entry_t *node = NULL;

-    assert(addr != NULL);
    DEBUG("nib: Allocating on-link node entry (addr = %s, iface = %u)\n",
          ipv6_addr_to_str(addr_str, addr, sizeof(addr_str)), iface);


this line will crash if addr == NULL

cgundogan · 2017-08-03T13:21:11Z

sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.c

@@ -341,6 +343,96 @@ _nib_dr_entry_t *_nib_drl_get_dr(void)
    return _prime_def_router;
 }

+_nib_offl_entry_t *_nib_offl_alloc(const ipv6_addr_t *next_hop, unsigned iface,
+                                   const ipv6_addr_t *pfx, unsigned pfx_len)


Fix your browser font :P

cgundogan · 2017-08-03T13:33:40Z

sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.c

+    _nib_offl_entry_t *dst = NULL;
+
+    assert((pfx != NULL) && (!ipv6_addr_is_unspecified(pfx)) &&
+           (pfx_len != 0) && (pfx_len <= 128));


just taste, but I like pfx_len > 0 more, because it's in harmony with px_len <= 128

Dito, don't know what came over me.

cgundogan · 2017-08-03T13:42:35Z

sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.h

-_nib_offl_entry_t *_nib_dst_alloc(const ipv6_addr_t *next_hop, unsigned iface,
-                                  const ipv6_addr_t *pfx, unsigned pfx_len);
+_nib_offl_entry_t *_nib_offl_alloc(const ipv6_addr_t *next_hop, unsigned iface,
+                                   const ipv6_addr_t *pfx, unsigned pfx_len);


Is fixed here, I know :P

cgundogan · 2017-08-03T13:45:33Z

sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.h

 * @return  NULL, if no space is left.
 */
 static inline _nib_offl_entry_t *_nib_dc_add(const ipv6_addr_t *next_hop,
                                             unsigned iface,
                                             const ipv6_addr_t *dst)
 {
-    return _nib_dst_add(next_hop, iface, dst, IPV6_ADDR_BIT_LEN, _DC);
+    assert(next_hop != NULL);


please also add assert(dst != NULL);

But it's checked in _nib_offl_alloc() (which is called by _nib_offl_add())...

That may be true for now, but what if this code changes with time and possibly by another developer?. IMO these asserts also have a documentational character.

cgundogan · 2017-08-03T13:49:31Z

tests/unittests/tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c

+ * interfaces and then tries to add another.
+ * Expected result: should return NULL
+ */
+static void test_nib_offl_alloc__no_space_left_diff_next_hop_iface(void)


I dont' think you need the above two test cases when you introduce this test case that combines both

I disagree, checking for behavior different addresses, different interfaces and different addresses and different interfaces are disjunct test-cases. One simple mix-up of && and || in the implementation would let fail at least one of them.

cgundogan · 2017-08-03T13:50:18Z

tests/unittests/tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c

+ * tries to add another.
+ * Expected result: should return NULL
+ */
+static void test_nib_offl_alloc__no_space_left_diff_next_hop_iface_pfx(void)


here the same, the above two test cases are not really necessary, are they?

See my comment above.

cgundogan · 2017-08-03T13:52:29Z

tests/unittests/tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c

+ * equal to the last.
+ * Expected result: should return not NULL (the last)
+ */
+static void test_nib_offl_alloc__success_duplicate(void)


So many test cases that are very similar .. can you collapse them? It is hard to maintain so many test cases in case of changes.

There are subtle differences that are not easily parameterized out. Ideally, this API shouldn't change much.

miri64 · 2017-08-08T12:09:55Z

Comments addressed.

miri64 · 2017-08-15T10:23:16Z

Found an error here, while debugging NDP-RD. Will fix.

Done: the problem was, if a Prefix List entry was allocated first and a neighbor cache entry to the same interface later, the allocation would create a new entry. The last two commits add a fix and a corresponding test for this.

miri64 · 2017-08-15T10:30:08Z

Oops, now the address isn't set ^^ will fix

miri64 · 2017-08-15T10:35:01Z

(fixed and ~~added~~ updated corresponding test)

miri64 · 2017-08-21T10:21:42Z

Ping @cgundogan? You promised to review last week :(

…IOT-OS#7222

…IOT-OS#7253

cgundogan

looks better now .. the goal draws nearer (:

cgundogan · 2017-10-06T06:31:39Z

sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.c

+            (_nib_onl_get_if(tmp_node) == iface) &&
+            ((next_hop == NULL) || ipv6_addr_is_unspecified(&tmp_node->ipv6) ||
+             (ipv6_addr_equal(next_hop, &tmp_node->ipv6))) &&
+            (ipv6_addr_match_prefix(&tmp->pfx, pfx) >= pfx_len)) {


That's a pretty ugly chain of conditions. Care to outsource into an inlined function or provide code documentation on what happens here?

cgundogan · 2017-10-06T07:07:44Z

sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.h

@@ -232,8 +237,6 @@ static inline void _nib_onl_set_if(_nib_onl_entry_t *node, unsigned iface)
 /**
 * @brief   Creates or gets an existing on-link entry by address
 *
- * @pre     `(addr != NULL)`.
- *
 * @param[in] addr  An IPv6 address. May not be NULL.


May ~~not~~ be NULL.

cgundogan · 2017-10-06T07:26:06Z

sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.h

 * @return  NULL, if no space is left.
 */
 static inline _nib_offl_entry_t *_nib_dc_add(const ipv6_addr_t *next_hop,
                                             unsigned iface,
                                             const ipv6_addr_t *dst)
 {
-    return _nib_dst_add(next_hop, iface, dst, IPV6_ADDR_BIT_LEN, _DC);
+    assert(next_hop != NULL);


That may be true for now, but what if this code changes with time and possibly by another developer?. IMO these asserts also have a documentational character.

miri64 · 2017-10-06T08:04:01Z

Addressed comments

cgundogan · 2017-10-06T09:06:22Z

@miri64 great, thanks for your patience! Please squash the commits and then let's murdock have some fun!

miri64 · 2017-10-06T09:12:12Z

Squashed

cgundogan · 2017-10-06T09:48:59Z

Murdock spits out some errors here.

miri64 · 2017-10-06T09:51:39Z

Murdock spits out some errors here.

Problem was the amended inline function requested in #7212 (comment). Fixed and squashed immediately.

cgundogan · 2017-10-06T10:02:39Z

Now murdock is happy (: GO

miri64 added GNRC Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Jun 20, 2017

miri64 assigned cgundogan Jun 20, 2017

miri64 requested a review from cgundogan June 20, 2017 13:28

smlng reviewed Jun 21, 2017

View reviewed changes

miri64 mentioned this pull request Jun 21, 2017

gnrc_ipv6_nib: doc: use attention tag instead of bold note #7218

Merged

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 21, 2017

Merge branch 'gnrc_ipv6_nib/feat/internal-offl' (RIOT-OS#7212) into HEAD

c15e4c8

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 21, 2017

Merge branch 'gnrc_ipv6_nib/feat/internal-offl' (RIOT-OS#7212) into HEAD

7561597

miri64 mentioned this pull request Jun 21, 2017

gnrc_ipv6_nib: add prefix list component #7222

Merged

2 tasks

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 22, 2017

Merge branch 'gnrc_ipv6_nib/feat/internal-offl' (RIOT-OS#7212) into R…

c8b822a

…IOT-OS#7222

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 23, 2017

Merge branch 'gnrc_ipv6_nib/feat/internal-offl' (RIOT-OS#7212) into HEAD

c091467

miri64 mentioned this pull request Jun 23, 2017

gnrc_ipv6_nib: add neighbor cache component #6988

Merged

aabadie modified the milestone: Release 2017.07 Jun 26, 2017

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 26, 2017

Merge branch 'gnrc_ipv6_nib/feat/internal-offl' (RIOT-OS#7212) into HEAD

af86afb

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 27, 2017

Merge branch 'gnrc_ipv6_nib/feat/internal-offl' (RIOT-OS#7212) into …

e8dff9e

…HEAD

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 27, 2017

Merge branch 'gnrc_ipv6_nib/feat/internal-offl' (RIOT-OS#7212) into R…

a21a5ab

…IOT-OS#7253

miri64 mentioned this pull request Jun 27, 2017

gnrc_ipv6_nib: add forwarding table component #7253

Merged

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 27, 2017

Merge branch 'gnrc_ipv6_nib/feat/internal-offl' (RIOT-OS#7212) into R…

6f7b466

…IOT-OS#7222

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 27, 2017

Merge branch 'gnrc_ipv6_nib/feat/internal-offl' (RIOT-OS#7212) into R…

0c25fa8

…IOT-OS#7253

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 29, 2017

Merge branch 'gnrc_ipv6_nib/feat/internal-offl' (RIOT-OS#7212) into …

640c0ab

…HEAD

miri64 mentioned this pull request Jun 29, 2017

gnrc_ipv6_nib: provide component to handle ABRO #7270

Merged

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 30, 2017

Merge branch 'gnrc_ipv6_nib/feat/internal-offl' (RIOT-OS#7212) into R…

f485452

…IOT-OS#7253

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 30, 2017

Merge branch 'gnrc_ipv6_nib/feat/internal-offl' (RIOT-OS#7212) into R…

82d0d4d

…IOT-OS#7222

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 30, 2017

Merge branch 'gnrc_ipv6_nib/feat/internal-offl' (RIOT-OS#7212) into R…

e3fee45

…IOT-OS#7222

aabadie modified the milestones: Release 2017.07, Release 2017.10 Jun 30, 2017

cgundogan requested changes Aug 3, 2017

View reviewed changes

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 21, 2017

Merge branch 'gnrc_ipv6_nib/feat/internal-offl' (RIOT-OS#7212) into R…

6d878ae

…IOT-OS#7222

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 21, 2017

Merge branch 'gnrc_ipv6_nib/feat/internal-offl' (RIOT-OS#7212) into R…

63dbcd7

…IOT-OS#7253

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 21, 2017

Merge branch 'gnrc_ipv6_nib/feat/internal-offl' (RIOT-OS#7212) into HEAD

25f6798

cgundogan requested changes Oct 6, 2017

View reviewed changes

cgundogan approved these changes Oct 6, 2017

View reviewed changes

cgundogan added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Oct 6, 2017

miri64 force-pushed the gnrc_ipv6_nib/feat/internal-offl branch from 34ab23a to 124d6c8 Compare October 6, 2017 09:12

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: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Oct 6, 2017

cgundogan 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 Oct 6, 2017

miri64 added 2 commits October 6, 2017 11:50

gnrc_ipv6_nib: provide internal implementation for off-link entries

f249c0c

tests: extend nib-internal unittests for off-link entry handling

c1f9b15

miri64 force-pushed the gnrc_ipv6_nib/feat/internal-offl branch from 124d6c8 to c1f9b15 Compare October 6, 2017 09:51

cgundogan merged commit c3d3e7d into RIOT-OS:master Oct 6, 2017

miri64 deleted the gnrc_ipv6_nib/feat/internal-offl branch October 6, 2017 10:02

gnrc_ipv6_nib: provide internal implementation for off-link entries #7212

gnrc_ipv6_nib: provide internal implementation for off-link entries #7212

Uh oh!

Conversation

miri64 commented Jun 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

cgundogan 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!

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 8, 2017

Uh oh!

miri64 commented Aug 15, 2017

Uh oh!

miri64 commented Aug 15, 2017

Uh oh!

miri64 commented Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Aug 21, 2017

Uh oh!

cgundogan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jun 20, 2017 •

edited

Loading

miri64 commented Aug 15, 2017 •

edited

Loading