gnrc_ndp2: Provide GNRC abstraction layer for NIB for sending by miri64 · Pull Request #7064 · RIOT-OS/RIOT

miri64 · 2017-05-16T08:04:26Z

For the new NIB simply using the old sending functions as I previously hoped (and why #6380 exists) isn't possible, since it is too tightly integrated in the old NDP. This PR (apart from a few code and API beatifications) is mostly a copy of the sending and building functions of gnrc_ndp, but it also introduces a set of tests.

The 2 is supposed to be removed as soon as the old NDP is removed

This PR is part of the network layer remodelling effort:

aabadie · 2017-06-23T08:41:36Z

@miri64, should we postponed this one ? I guess yes.

miri64 · 2017-06-23T09:17:10Z

The new ND wouldn't make it to 2017.07 anyway, so I guess yes.

cgundogan

first round ...

cgundogan · 2017-08-04T06:38:19Z

sys/include/net/gnrc/ndp2.h

+ */
+#ifndef GNRC_NETTYPE_NDP2
+# if    defined(MODULE_GNRC_IPV6) || DOXYGEN
+#  define GNRC_NETTYPE_NDP2 (GNRC_NETTYPE_IPV6)


Is this conditional define necessary? When is NDP being used witout IPv6?

In the tests :P

so, maybe mention this fact here? It's really confusing to bend things in the implementation that are oly needed for testing.

cgundogan · 2017-08-04T06:39:11Z

sys/include/net/gnrc/ndp2.h

+ *
+ * @pre `(tgt != NULL) && !ipv6_addr_is_multicast(tgt)`
+ *
+ * @see [RFC 4861, section 4.3](https://tools.ietf.org/html/rfc4861#section-4.3")


cgundogan · 2017-08-04T06:40:19Z

sys/include/net/gnrc/ndp2.h

+ *                      For and unsolicited advertisement, the address whose
+ *                      link-layer address has changed.
+ *                      May not be NULL and **MUST NOT** be multicast.
+ * @param[in] flags     Neighbor advertisement flags:


Not sure, what you mean?

shift everything starting from Neighbor one character to the right. That's what I mean (:

Hm, appears to be a problem in my browser then. It's shifted wrongly here. There's no tab or thin whitespace there, right?

cgundogan · 2017-08-04T06:44:32Z

sys/include/net/gnrc/ndp2.h

+ *
+ * @param[in] cur_hl        Default hop limit for outgoing IP packets, 0 if
+ *                          unspecified by this router.
+ * @param[in] flags         Flags as defined above.


Not sure, what you mean?

cgundogan · 2017-08-04T06:45:21Z

sys/include/net/gnrc/ndp2.h

+ * @param[in] pref_ltime    Length of time in seconds that addresses using
+ *                          @p prefix remain prefered. UINT32_MAX represents
+ *                          infinity.
+ * @param[in] flags         Flags as defined above.


Not sure, what you mean?

cgundogan · 2017-08-04T07:17:12Z

sys/net/gnrc/network_layer/ndp2/gnrc_ndp2.c

+            pkt = hdr;
+        }
+    }
+    /* TODO: also check if the node provides proxy servies for tgt */


/servies/sevices/

cgundogan · 2017-08-04T07:18:59Z

sys/net/gnrc/network_layer/ndp2/gnrc_ndp2.c

+    /* TODO: also check if the node provides proxy servies for tgt */
+    if ((pkt != NULL) &&
+        (!gnrc_ipv6_netif_addr_is_non_unicast(tgt) || supply_tl2a)) {
+        /* TL2A is not supplied and tgt is not anycast */


/TL2AO/TLLAO/

See #7064 (comment)

cgundogan · 2017-08-04T07:20:16Z

sys/net/gnrc/network_layer/ndp2/gnrc_ndp2.c

+    DEBUG("ndp2: send router solicitation (iface: %" PRIkernel_pid
+          ", dst: %s)\n", netif->pid,
+          ipv6_addr_to_str(addr_str, dst, sizeof(addr_str)));
+    /* add SLLAO => check if there is a fitting source address to target */


note: here you called it SLLAO

That's an error :P

cgundogan · 2017-08-04T07:21:56Z

sys/net/gnrc/network_layer/ndp2/gnrc_ndp2.c

+    bool try_long = false;
+    int res;
+    uint16_t l2src_len;
+    /* maximum address length that fits into a minimum length (8) S/TL2A option */


#7064 (comment)

cgundogan · 2017-08-04T07:31:10Z

tests/gnrc_ndp2/main.c

+                                                        TEST_RETRANS_TIMER,
+                                                        NULL)));
+    TEST_ASSERT(gnrc_pktbuf_is_sane());
+    /* check packet meta-data */


please provide a macro or inline function for this meta-data check to reduce code redundancy

(this will make the tests harder to debug if they fail, but okay).

miri64 · 2017-08-18T17:06:03Z

Needed to change the API a little bit. I realized that I need access to the NIB internals for the ARO, so I removed it from the nbr_sol_send() function.

cgundogan

getting there.

cgundogan · 2017-10-06T07:40:39Z

sys/include/net/gnrc/ndp2.h

+ */
+
+/**
+ * @defgroup    net_gnrc_ndp2   IPv6 Neighbor discovery (v2)


Uppercase d for Discovery, otherwise lowercase n for Neighbor

cgundogan · 2017-10-06T07:51:16Z

sys/net/gnrc/network_layer/ndp2/gnrc_ndp2.c

+    assert((tgt != NULL) && !ipv6_addr_is_multicast(tgt));
+    DEBUG("ndp2: building neighbor solicitation message\n");
+    pkt = gnrc_icmpv6_build(options, ICMPV6_NBR_SOL, 0, sizeof(ndp_nbr_sol_t));
+    if (pkt != NULL) {


For debugging purposes, I would rather like an early exit in case of pkt == NULL and an appropriate DEBUG statement for that. Here and all following functions.

Can we decide on one style please? I think in the end the given style will lead to cleaner code and smaller code size.

I can add an else-statement at the end with the debug message as a compromise.

The given style very quickly leads to nested if blocks. That's already the case for a lot of places in the existing NDP code. I don't think that an early exit leads to bigger code sizes, but definitely makes the code more readable by removing one block of indentation.

Honestly it's a matter of taste. I could change it, but I remember that @OlegHahm was blocking early exits in the review of the old NDP. So which taste should I follow now?

then let's hear @OlegHahm's opinion about this.

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

sys/net/gnrc/network_layer/ndp2/gnrc_ndp2.c

+
+static inline size_t _ceil8(uint8_t length)
+{
+    /* NDP options use units of 8 byte for there length field, so round up */


s/there/their/

cgundogan · 2017-10-06T08:44:41Z

sys/net/gnrc/network_layer/ndp2/gnrc_ndp2.c

+        memcpy(&real_dst, dst, sizeof(real_dst));
+        adv_flags |= NDP_NBR_ADV_FLAGS_S;
+    }
+    /* add SL2AO based on target address */


I don't know what you mean by alternative spellings. SLLAO is the official abbreviation for Source Link-Layer Address Option (cf. Appendix F of RFC4861):

o Clarified router behavior when receiving a Router Solicitation
without Source Link-Layer Address Option (SLLAO).

miri64 · 2017-10-06T09:31:14Z

Addressed comments.

miri64 · 2017-10-06T09:36:07Z

Reply to #7064 (comment) in #7064 (comment) btw (why can't it show up under your reply above oO?)

cgundogan · 2017-10-06T09:42:16Z

Latest commit is good, please squash!

miri64 · 2017-10-06T09:43:50Z

Squashed

miri64 · 2017-10-06T10:11:35Z

Fixed issue Murdock had and amended.

miri64 · 2017-10-06T10:25:02Z

Another try :-)

miri64 · 2017-10-06T11:10:37Z

And another -.-

miri64 · 2017-10-06T11:29:47Z

Murdock finally liked it \o/

cgundogan · 2017-10-06T11:33:32Z

GO!

miri64 · 2017-10-06T11:35:26Z

GO!

Not waiting for @OlegHahm's opinion on early exits? Oo.... but okay you are the reviewer, I will not complain :D

cgundogan · 2017-10-06T11:39:23Z

Not waiting for @OlegHahm's opinion on early exits? Oo.... but okay you are the reviewer, I will not complain :D

dammit :P

miri64 added Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. GNRC Area: network Area: Networking labels May 16, 2017

miri64 assigned cgundogan May 16, 2017

miri64 requested a review from cgundogan May 16, 2017 08:04

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 16, 2017

miri64 mentioned this pull request May 16, 2017

gnrc_ndp: add adapters for NIB-based implementation #6380

Closed

miri64 added a commit to miri64/RIOT that referenced this pull request May 16, 2017

Merge branch 'gnrc_ndp2/api/initial' (RIOT-OS#7064) into HEAD

1d99840

miri64 mentioned this pull request May 16, 2017

nib: implement public NIB functions up to link-local AR #7014

Merged

2 tasks

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

Merge branch 'gnrc_ndp2/api/initial' (RIOT-OS#7064) into HEAD

64020e4

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

Merge branch 'gnrc_ndp2/api/initial' (RIOT-OS#7064) into RIOT-OS#7014

671feac

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

Merge branch 'gnrc_ndp2/api/initial' (RIOT-OS#7064) into RIOT-OS#7014

9569682

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

Merge branch 'gnrc_ndp2/api/initial' (RIOT-OS#7064) into RIOT-OS#7014

be2cd11

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

Merge branch 'gnrc_ndp2/api/initial' (RIOT-OS#7064) into RIOT-OS#7014

44b0731

aabadie added this to the Release 2017.10 milestone Jun 23, 2017

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

Merge branch 'gnrc_ndp2/api/initial' (RIOT-OS#7064) into RIOT-OS#7014

2b98b0b

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

Merge branch 'gnrc_ndp2/api/initial' (RIOT-OS#7064) into RIOT-OS#7014

24aadc9

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

Merge branch 'gnrc_ndp2/api/initial' (RIOT-OS#7064) into RIOT-OS#7014

8b4a6ee

cgundogan requested changes Aug 4, 2017

View reviewed changes

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

Merge branch 'gnrc_ndp2/api/initial' (RIOT-OS#7064) into RIOT-OS#7014

351101c

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

Merge branch 'gnrc_ndp2/api/initial' (RIOT-OS#7064) into RIOT-OS#7014

6f39a3b

cgundogan requested 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_ndp2/api/initial branch from f7830ca to e3b7f43 Compare October 6, 2017 09:43

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

gnrc_ndp2: Provide GNRC abstraction layer for NIB for sending

08447ed

miri64 force-pushed the gnrc_ndp2/api/initial branch from e3b7f43 to d9a884e Compare October 6, 2017 10:11

miri64 force-pushed the gnrc_ndp2/api/initial branch from fb73e6e to 283cc51 Compare October 6, 2017 10:25

tests: provide tests for gnrc_ndp2

78474fc

miri64 force-pushed the gnrc_ndp2/api/initial branch from 283cc51 to 78474fc Compare October 6, 2017 11:10

cgundogan approved these changes Oct 6, 2017

View reviewed changes

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

miri64 deleted the gnrc_ndp2/api/initial branch October 6, 2017 11:34

Conversation

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

Uh oh!

Uh oh!

aabadie commented Jun 23, 2017

Uh oh!

miri64 commented Jun 23, 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!

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 18, 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!

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 Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 May 16, 2017 •

edited

Loading

cgundogan Oct 6, 2017 •

edited

Loading