fib: changed handling of the net prefix by the FIB #4279

BytesGalore · 2015-11-16T11:31:15Z

Until now the prefix length for a given destination has been determined automatically by the FIB.
This PR changes it to be provided as upper byte in the global_flags of an entry.
Inspired by the IP scheme, the number of significant bits is stored in the designated most upper byte of the flags.

Rationale:
Before this change there was for instance no way to set a >116 prefix for a private subnet like 2001::1234:0000.
The API CHANGE Label belongs only to the reserved upper byte of the global_flags of a FIB entry used as storage for the prefix bits.

cgundogan · 2015-11-16T11:55:03Z

sys/net/gnrc/routing/rpl/gnrc_rpl_dodag.c

@@ -243,7 +243,7 @@ void gnrc_rpl_parent_update(gnrc_rpl_dodag_t *dodag, gnrc_rpl_parent_t *parent)
            kernel_pid_t if_id;
            if ((if_id = gnrc_ipv6_netif_find_by_addr(NULL, &all_RPL_nodes)) != KERNEL_PID_UNDEF) {
                fib_add_entry(&gnrc_ipv6_fib_table, if_id, def.u8, sizeof(ipv6_addr_t),
-                              (FIB_FLAG_NET_PREFIX | 0x0),
+                              (0x00 << FIB_FLAG_NET_PREFIX_SHIFT | 0x0),


0x00 << FIB_FLAG_NET_PREFIX_SHIFT == 0`?

yup, made it to keep all cases equal.
(0x00 << FIB_FLAG_NET_PREFIX_SHIFT) flag for IPV6_ADDR_UNSPECIFIED is now considered as default route by the FIB (inspired by ::/0 notation).

Why don't you just write 0x0 instead of 0x00 << FIB_FLAG_NET_PREFIX_SHIFT | 0x0 ?

I changed it :)

cgundogan · 2015-11-16T15:22:45Z

I am just wondering: aren't 7 bits sufficient for storing the prefix?

EDIT: nvm, since it is [0-128] it won't fit :/

BytesGalore · 2015-11-16T15:27:32Z

For IPv6 yes.
But I aim the FIB to be more flexible then just support up to IPv6 :)

cgundogan · 2015-11-16T15:49:26Z

sys/net/gnrc/routing/rpl/gnrc_rpl_dodag.c

@@ -243,7 +243,7 @@ void gnrc_rpl_parent_update(gnrc_rpl_dodag_t *dodag, gnrc_rpl_parent_t *parent)
            kernel_pid_t if_id;
            if ((if_id = gnrc_ipv6_netif_find_by_addr(NULL, &all_RPL_nodes)) != KERNEL_PID_UNDEF) {
                fib_add_entry(&gnrc_ipv6_fib_table, if_id, def.u8, sizeof(ipv6_addr_t),
-                              (FIB_FLAG_NET_PREFIX | 0x0),
+                              0x0,


you also need to adjust line 308 otherwise it won't compile

cgundogan · 2015-11-16T15:55:58Z

RIOT/sys/net/network_layer/fib/fib.c:1535:28: error: format '%d' expects argument
                     of type 'int', but argument 2 has type 'uint32_t
                     {aka long unsigned int}' [-Werror=format=]
                     printf("N /%d ", (prefix >> FIB_FLAG_NET_PREFIX_SHIFT));

BytesGalore · 2015-11-16T16:01:01Z

arrrgh, done.

BytesGalore · 2015-11-24T05:51:09Z

rebased

cgundogan · 2015-11-24T17:15:09Z

I am testing this change again

Yonezawa-T2 · 2015-12-11T07:39:42Z

sys/shell/commands/sc_fib.c

@@ -74,15 +74,33 @@ static void _fib_usage(int info)

 static void _fib_add(const char *dest, const char *next, kernel_pid_t pid, uint32_t lifetime)
 {
-    unsigned char *dst = (unsigned char *)dest;
-    size_t dst_size = (strlen(dest));
+    uint32_t prefix = 0;


If prefix is omitted, should it be 0 or 128?

The FIB omits the prefix if the specified byte in the falg is 0 or the length of the address in bits.
So 0 omits the prefix for any used address type, e.g. IPv6, IPv4, ..., and 128 omits an IPv6 prefix.

cgundogan · 2015-12-11T07:45:15Z

Sorry for delaying this so much. I will be able to look at it again at the end of next week.

BytesGalore · 2015-12-11T08:06:51Z

@cgundogan np :) , the release is/was way more important than this PR.

BytesGalore · 2016-03-29T18:15:37Z

@cgundogan nice thx for reviewing :)

OlegHahm · 2016-03-29T20:54:57Z

sys/universal_address/universal_address.c

+    /* compare up to fist distinct byte, pretty clumsy method for now */
+    int idx = -1;
+    bool test_all_zeros = true;
+    for( size_t i = 0; i < entry->address_size; i++) {


superfluous white-space before size_t.

here and below

PeterSchmerzl · 2016-03-29T21:24:48Z

ACK

OlegHahm · 2016-03-29T21:45:51Z

sys/net/network_layer/fib/fib.c

+                                                    & FIB_FLAG_NET_PREFIX_MASK) >> FIB_FLAG_NET_PREFIX_SHIFT;
+
+                        if ((match_size >= global_prefix_len) &&
+                            ((prefix_size == 0) || (match_size > prefix_size))) {


~~prefix_size is initialized as 0 and not set anywhere before to something else.~~

OlegHahm · 2016-03-29T21:55:18Z

ACK - apart from minor comments.

BytesGalore · 2016-03-30T06:21:34Z

sys/universal_address/universal_address.c

-                for(; i < entry->address_size; ++i) {
-                    if( entry->address[i] != 0 ) {
-                        ret = 1;
+                for ( ; i < entry->address_size; ++i) {


I intentionally added the whitespace before the first ; in the for statement because I tend to overlook them in such cases.

Yeah, that's okay here.

BytesGalore · 2016-03-30T06:32:42Z

@OlegHahm I guess I addressed all your comments, thx for reviewing :)
I made a separate companion commit for the doxy/define changes in universal_address

@PeterSchmerzl welcome on the code side of the force, and thx for your ACK

OlegHahm · 2016-03-30T07:26:16Z

Murdock has some complaints.

BytesGalore · 2016-03-30T07:52:09Z

ok, I think I got 'em

cgundogan · 2016-03-30T08:03:14Z

sys/include/universal_address.h

- *         -ENOENT if the given adresses do not match
+ * @return UNIVERSAL_ADDRESS_EQUAL if the entries are equal
+ * @return UNIVERSAL_ADDRESS_MATCHING_PREFIX if the entry matches to a certain prefix 
+ *                                           (trailing '0's in *entry)


s/*entry/@p entry/

Until now the prefix length has been determined automatically by the FIB This PR changes it to be provided as msb(yte) in the global_flags of an entry

cgundogan · 2016-03-30T08:19:48Z

sys/include/universal_address.h

- *         -ENOENT if the given adresses do not match
+ * @return UNIVERSAL_ADDRESS_EQUAL if the entries are equal
+ * @return UNIVERSAL_ADDRESS_MATCHING_PREFIX if the entry matches to a certain prefix
+ *                                           (trailing '0's in *entry)


s/*entry/@p entry/

* added and adjusted doxy for the new defined return values * stripped whitespaces for statements in the compare functions

cgundogan · 2016-03-30T09:19:47Z

Okay, I did a last test on the iotlab testbed with routing over 3 hops. Worked fine, my ACK upholds, @OlegHahm has already acked and Murdock is green => GO

cgundogan · 2016-03-30T09:19:56Z

Congrats!

BytesGalore · 2016-03-30T09:23:48Z

nice thx again :D

BytesGalore added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Nov 16, 2015

BytesGalore assigned cgundogan Nov 16, 2015

cgundogan reviewed Nov 16, 2015
View reviewed changes

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

BytesGalore force-pushed the fib_prefix_handling_change branch from 75123b0 to 2464b25 Compare November 16, 2015 12:16

cgundogan reviewed Nov 16, 2015
View reviewed changes

BytesGalore force-pushed the fib_prefix_handling_change branch from 2feca87 to fbcd04a Compare November 24, 2015 05:50

BytesGalore mentioned this pull request Nov 24, 2015

fib: fix output of fibroute #4322

Merged

OlegHahm modified the milestone: Release 2016.03 Dec 7, 2015

BytesGalore force-pushed the fib_prefix_handling_change branch from fbcd04a to fb45f8c Compare December 9, 2015 16:14

BytesGalore mentioned this pull request Dec 11, 2015

shell: updated fibroute command to support prefix route #4464

Closed

Yonezawa-T2 reviewed Dec 11, 2015
View reviewed changes

Yonezawa-T2 mentioned this pull request Dec 16, 2015

gnrc_ndp: add external routers to FIB when RPL is enabled #4489

Closed

BytesGalore force-pushed the fib_prefix_handling_change branch from fb45f8c to db736d9 Compare January 19, 2016 09:06

Yonezawa-T2 mentioned this pull request Jan 21, 2016

gnrc_rpl_control_messages: propagete prefix route via DAO message #4661

Merged

OlegHahm reviewed Mar 29, 2016
View reviewed changes

BytesGalore force-pushed the fib_prefix_handling_change branch 3 times, most recently from f559afa to 79809ff Compare March 30, 2016 06:19

BytesGalore reviewed Mar 30, 2016
View reviewed changes

BytesGalore added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 30, 2016

BytesGalore force-pushed the fib_prefix_handling_change branch 2 times, most recently from 90930fe to 2f83aab Compare March 30, 2016 07:50

cgundogan reviewed Mar 30, 2016
View reviewed changes

fib: changed handling of the net prefix

1bc9c3b

Until now the prefix length has been determined automatically by the FIB This PR changes it to be provided as msb(yte) in the global_flags of an entry

BytesGalore force-pushed the fib_prefix_handling_change branch from 2f83aab to 52759f7 Compare March 30, 2016 08:18

cgundogan reviewed Mar 30, 2016
View reviewed changes

universal_address: replaced returned literals by defined constants

38d5fc2

* added and adjusted doxy for the new defined return values * stripped whitespaces for statements in the compare functions

BytesGalore force-pushed the fib_prefix_handling_change branch from cb09bcb to 38d5fc2 Compare March 30, 2016 08:29

cgundogan merged commit 9dbfdca into RIOT-OS:master Mar 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fib: changed handling of the net prefix by the FIB #4279

fib: changed handling of the net prefix by the FIB #4279

BytesGalore commented Nov 16, 2015

cgundogan Nov 16, 2015

BytesGalore Nov 16, 2015

cgundogan Nov 16, 2015

BytesGalore Nov 16, 2015

cgundogan commented Nov 16, 2015

BytesGalore commented Nov 16, 2015

cgundogan Nov 16, 2015

BytesGalore Nov 16, 2015

cgundogan commented Nov 16, 2015

BytesGalore commented Nov 16, 2015

BytesGalore commented Nov 24, 2015

cgundogan commented Nov 24, 2015

Yonezawa-T2 Dec 11, 2015

BytesGalore Dec 11, 2015

cgundogan commented Dec 11, 2015

BytesGalore commented Dec 11, 2015

BytesGalore commented Mar 29, 2016

OlegHahm Mar 29, 2016

OlegHahm Mar 29, 2016

PeterSchmerzl commented Mar 29, 2016

OlegHahm Mar 29, 2016

OlegHahm commented Mar 29, 2016

BytesGalore Mar 30, 2016

OlegHahm Mar 30, 2016

BytesGalore commented Mar 30, 2016

OlegHahm commented Mar 30, 2016

BytesGalore commented Mar 30, 2016

cgundogan Mar 30, 2016

cgundogan Mar 30, 2016

BytesGalore Mar 30, 2016

cgundogan commented Mar 30, 2016

cgundogan commented Mar 30, 2016

BytesGalore commented Mar 30, 2016

fib: changed handling of the net prefix by the FIB #4279

fib: changed handling of the net prefix by the FIB #4279

Conversation

BytesGalore commented Nov 16, 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

cgundogan commented Nov 16, 2015

BytesGalore commented Nov 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgundogan commented Nov 16, 2015

BytesGalore commented Nov 16, 2015

BytesGalore commented Nov 24, 2015

cgundogan commented Nov 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgundogan commented Dec 11, 2015

BytesGalore commented Dec 11, 2015

BytesGalore commented Mar 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterSchmerzl commented Mar 29, 2016

Choose a reason for hiding this comment

OlegHahm commented Mar 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BytesGalore commented Mar 30, 2016

OlegHahm commented Mar 30, 2016

BytesGalore commented Mar 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgundogan commented Mar 30, 2016

cgundogan commented Mar 30, 2016

BytesGalore commented Mar 30, 2016