gnrc_netif: add gnrc_netif_ipv6_add_prefix() & helper functions by benpicco · Pull Request #16672 · RIOT-OS/RIOT

benpicco · 2021-07-22T14:28:20Z

Contribution description

Code from dhcpv6_client_conf_prefix() can be useful in other situations where we want to configure a prefix on an interface.
Move it to a common place and also use it for uhcp.

Testing procedure

examples/gnrc_border_router still gets a valid prefix

via UHCP

Iface  6  HWaddr: AE:8D:C2:75:23:DE 
          L2-PDU:1500  MTU:1500  HL:64  RTR  
          Source address length: 6
          Link type: wired
          inet6 addr: fe80::ac8d:c2ff:fe75:23de  scope: link  VAL
          inet6 addr: fe80::2  scope: link  VAL
          inet6 group: ff02::2
          inet6 group: ff02::1
          inet6 group: ff02::1:ff75:23de
          inet6 group: ff02::1:ff00:2
          
Iface  5  HWaddr: 49:05  Channel: 26  Page: 0  NID: 0x23  PHY: O-QPSK 
          
          Long HWaddr: 00:04:25:19:18:01:C9:05 
           TX-Power: 0dBm  State: IDLE  max. Retrans.: 3  CSMA Retries: 4 
          AUTOACK  ACK_REQ  CSMA  L2-PDU:102  MTU:1280  HL:64  RTR  
          RTR_ADV  6LO  IPHC  
          Source address length: 8
          Link type: wireless
          inet6 addr: fe80::204:2519:1801:c905  scope: link  VAL
          inet6 addr: 2001:db8::204:2519:1801:c905  scope: global  VAL
          inet6 group: ff02::2
          inet6 group: ff02::1
          inet6 group: ff02::1:ff01:c905

Issues/PRs references

split off #16536

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

miri64 · 2021-07-22T17:04:47Z

sys/net/gnrc/network_layer/ipv6/util.c

+           (ipv6_addr_match_prefix(&ctx->prefix, prefix) >= prefix_len);
+}
+
+static void _update_6ctx(const ipv6_addr_t *prefix, uint8_t prefix_len,


Setting the context is typically only something you want to do on the border router, so putting that into the generic code seem misplaced.

This is already behind a if (IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_6LBR))

OK, then this should be documented...

miri64 · 2021-07-22T17:07:55Z

sys/net/gnrc/network_layer/ipv6/util.c

+            }
+            (void)gnrc_ipv6_nib_abr_add(&addr);
+        }
+        if (IS_USED(MODULE_GNRC_RPL)) {


Same for the RPL root. You do not necessarily want to add one. On every node you want to set a prefix for...

Should I just move that to a separate helper function then?

benpicco · 2021-07-23T10:14:39Z

sys/net/gnrc/network_layer/ipv6/util.c

+            }
+            (void)gnrc_ipv6_nib_abr_add(&addr);
+        }
+        if (IS_USED(MODULE_GNRC_RPL)) {


So

Suggested change

if (IS_USED(MODULE_GNRC_RPL)) {

if (IS_USED(MODULE_GNRC_RPL) &&

IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_6LBR)) {

?

A border router does not necessarily has to be a RPL root or a RPL root must not be a border router. These lines purely exist to make our gnrc_border_router example happy (where the border router is assumed to be a RPL root), so this should not be here, but in a 6LBR-specific module (like it is the case with DHCPv6 and with ethos somewhat, because ethos is our thing anyways).

Should I make a separate helper function for RPL then? This is after all already duplicated in both gnrc_uhcpc.c and dhcpv6/client.c.

Yes, I think this can be a nice convenience function within the gnrc_rpl function. The rpl root shell command probably can also benefit from that.

miri64 · 2021-07-23T13:46:58Z

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

+ * @param[in] pref      Preferred lifetime of the prefix delegation.
+ * @param[in] tentative Add new prefix as tentative.
+ */
+void gnrc_util_conf_prefix(gnrc_netif_t *netif, const ipv6_addr_t *pfx,


This might surprise you, but why not move it to gnrc_netif_conf_prefix() in net/gnrc/netif/internal.h or maybe net/gnrc/netif/helper.h? Since in this case this is all about interface configuration (setting a prefix in the NIB for an interface, setting an address for that prefix on an interface, ...), I think in this case it fits there.

miri64 · 2021-07-23T13:47:30Z

sys/net/gnrc/network_layer/ipv6/util.c

+           (ipv6_addr_match_prefix(&ctx->prefix, prefix) >= prefix_len);
+}
+
+static void _update_6ctx(const ipv6_addr_t *prefix, uint8_t prefix_len,


OK, then this should be documented...

miri64 · 2021-07-23T13:48:43Z

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

+
+/**
+ * @brief   Configures a prefix delegation lease that is provided by the server.
+ *


Please add some details on what this does entail. From a user perspective: there is already gnrc_ipv6_nib_pl_add() so, why is this better / more feature-some?

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

benpicco · 2021-07-23T16:03:42Z

Do you have an explanation why the DHCPv6 client adds the prefix as tentative while UHCP just adds it as valid?
If we got it from DHCPv6 we couldn't we assume it to be valid too?

miri64 · 2021-07-27T20:49:42Z

Do you have an explanation why the DHCPv6 client adds the prefix as tentative while UHCP just adds it as valid?
If we got it from DHCPv6 we couldn't we assume it to be valid too?

I think it's just a bug that the DHCPv6 client adds the [address?, prefixes do not have a tentative state] as tentative. Should we maybe fix this in master as well, before merging this PR?

miri64 · 2021-07-28T06:06:23Z

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

+    if (inst) {
+        gnrc_rpl_instance_remove(inst);
+    }
+    gnrc_rpl_root_init(CONFIG_GNRC_RPL_DEFAULT_INSTANCE, &addr, false, false);


Where is addr coming from?

I added gnrc_netif_ipv6_get_global() to get it

Why not just provide it as a parameter dodag_id? Guessing the address the DODAG should be opened on (is the one returned by get_global() really the one configured by the config protocol) seems like a bad idea. Not to mention that I do not see much use in such a guessing function in general.

True and since gnrc_netif_ipv6_addr_add_internal() returns the index of the new address, we can just make use of that.

sys/net/gnrc/application_layer/uhcpc/gnrc_uhcpc.c

miri64

Last minor comment. After that this is good to go.

sys/net/gnrc/netif/gnrc_netif_util.c

miri64 · 2021-07-29T09:30:49Z

sys/net/gnrc/netif/gnrc_netif.c

+#if IS_USED(MODULE_GNRC_SIXLOWPAN_CTX)
+static bool _ctx_match(const gnrc_sixlowpan_ctx_t *ctx,
+                       const ipv6_addr_t *prefix, uint8_t prefix_len)
+{
+    return (ctx != NULL) &&
+           (ctx->prefix_len == prefix_len) &&
+           (ipv6_addr_match_prefix(&ctx->prefix, prefix) >= prefix_len);
+}
+
+/* configure 6LoWPAN compression context */
+static void _update_6ctx(const ipv6_addr_t *prefix, uint8_t prefix_len,
+                         uint32_t valid)
+{
+    gnrc_sixlowpan_ctx_t *ctx = gnrc_sixlowpan_ctx_lookup_addr(prefix);
+    uint8_t cid = 0;
+
+    if (!_ctx_match(ctx, prefix, prefix_len)) {
+        /* While the context is a prefix match, the defined prefix within the
+         * context does not match => use new context */
+        ctx = NULL;
+    }
+    else {
+        cid = ctx->flags_id & GNRC_SIXLOWPAN_CTX_FLAGS_CID_MASK;
+    }
+    /* find first free context ID */
+    if (ctx == NULL) {
+        while (((ctx = gnrc_sixlowpan_ctx_lookup_id(cid)) != NULL) &&
+               !_ctx_match(ctx, prefix, prefix_len)) {
+            cid++;
+        }
+    }
+    if (cid < GNRC_SIXLOWPAN_CTX_SIZE) {
+        DEBUG("GNRC util: add compression context %u for prefix %s/%u\n", cid,
+              ipv6_addr_to_str(addr_str, prefix, sizeof(addr_str)),
+              prefix_len);
+        gnrc_sixlowpan_ctx_update(cid, (ipv6_addr_t *)prefix, prefix_len,
+                                  valid / (60 * MS_PER_SEC),
+                                  true);
+    }
+}
+#else
+static void _update_6ctx(const ipv6_addr_t *prefix, uint8_t prefix_len,
+                         uint32_t valid)
+{
+    (void)prefix;
+    (void)prefix_len;
+    (void)valid;
+}
+#endif


Last thought (and I am aware that this is more or less asking you to clean up my mess 😅): Would it make sense to move these as convenience functions to net/gnrc/sixlowpan/ctx.h?

Are those used anywhere else?

Not really, but they are more about manipulating the context store than changing a gnrc_netif (they don't even use the gnrc_netif_t type anywhere) + you surround them with a module-specific #ifdef anywhere, so it seems cleaner to put them in the module they belong to ;-).

You can make them static inline, so using them should not change the binary.

DEBUG("GNRC util: add compression context %u for prefix %s/%u\n", cid, ipv6_addr_to_str(addr_str, prefix, sizeof(addr_str)), prefix_len);

won't work as static inline if there is no global addr_str buffer

I can move it to gnrc_sixlowpan_ctx.c

DEBUG("GNRC util: add compression context %u for prefix %s/%u\n", cid, ipv6_addr_to_str(addr_str, prefix, sizeof(addr_str)), prefix_len);

won't work as static inline if there is no global addr_str buffer

Do we really need it?

If you think it is valuable, we can also add a DEBUG statement here

DEBUG("GNRC util: attempting to add compression context for prefix %s/%u\n", ipv6_addr_to_str(addr_str, prefix, sizeof(addr_str)), prefix_len);

Ok, moved it there and added some documentation.

miri64

Some post-review cleanup of this PR is also required:

Some debug messages still refer to GNRC util, please adapt for the respective modules the functions now reside in
Title of this PR (and possibly commit summaries) should be adapted to new status quo. How about 'gnrc: several convenience functions for sub-net bootstraping' as PR title and then single commits for each new function, describing what they do respectively?

miri64

We can do the module magic for gnrc_sixlowpan_ctx as a C-conditional now.

sys/include/net/gnrc/sixlowpan/ctx.h

miri64

ACK. The examples/gnrc_border_router example using gnrc_rpl still works and everything is configured as expected.

With UHCP

----> ethos: sending hello.
----> ethos: activating serial pass through.
----> ethos: hello reply received
uhcp_client(): no reply received
uhcp_client(): sending REQ...
got packet from fe80::e0bc:7dff:fecb:f54f port 49028
uhcp: push from fe80::e0bc:7dff:fecb:f54f:49028 prefix=2001:db8::/64
ifconfig
ifconfig
Iface  6  HWaddr: AE:FF:4D:A3:CD:6C 
          L2-PDU:1500  MTU:1500  HL:64  RTR  
          Source address length: 6
          Link type: wired
          inet6 addr: fe80::acff:4dff:fea3:cd6c  scope: link  VAL
          inet6 addr: fe80::2  scope: link  VAL
          inet6 group: ff02::2
          inet6 group: ff02::1
          inet6 group: ff02::1:ffa3:cd6c
          inet6 group: ff02::1:ff00:2
          
Iface  5  HWaddr: 2E:82  Channel: 26  Page: 0  NID: 0x23  PHY: O-QPSK 
          
          Long HWaddr: 00:04:25:19:18:01:AE:82 
           TX-Power: 0dBm  State: IDLE  max. Retrans.: 3  CSMA Retries: 4 
          AUTOACK  ACK_REQ  CSMA  L2-PDU:102  MTU:1280  HL:64  RTR  
          RTR_ADV  6LO  IPHC  
          Source address length: 8
          Link type: wireless
          inet6 addr: fe80::204:2519:1801:ae82  scope: link  VAL
          inet6 addr: 2001:db8::204:2519:1801:ae82  scope: global  VAL
          inet6 group: ff02::2
          inet6 group: ff02::1
          inet6 group: ff02::1:ff01:ae82
          inet6 group: ff02::1a
          
> 6ctx
6ctx
cid|prefix                                     |C|ltime
-----------------------------------------------------------
  0|                             2001:db8::/64 |1| 1092min
> nib abr
nib abr
2001:db8::204:2519:1801:ae82 v0 expires 10000min
> rpl
rpl
instance table:	[X]	
parent table:	[ ]	[ ]	[ ]	

instance [0 | Iface: 5 | mop: 2 | ocp: 0 | mhri: 256 | mri 0]
	dodag [2001:db8::204:2519:1801:ae82 | R: 256 | OP: Router | PIO: on | TR(I=[8,20], k=10, c=0)]
>

With DHCPv6

----> ethos: sending hello.
----> ethos: activating serial pass through.
----> ethos: hello reply received
2021-07-29 15:48:32.324 INFO  [kea-dhcp6.dhcp6/230443.140459950431936] DHCP6_STARTING Kea DHCPv6 server version 1.9.9 (development) starting
2021-07-29 15:48:32.324 WARN  [kea-dhcp6.dhcp6/230443.140459950431936] DHCP6_DEVELOPMENT_VERSION This software is a development branch of Kea. It is not recommended for production use.
ifconfig
ifconfig
Iface  6  HWaddr: AE:FF:4D:A3:CD:6C 
          L2-PDU:1500  MTU:1500  HL:64  RTR  
          Source address length: 6
          Link type: wired
          inet6 addr: fe80::acff:4dff:fea3:cd6c  scope: link  VAL
          inet6 addr: fe80::2  scope: link  VAL
          inet6 group: ff02::2
          inet6 group: ff02::1
          inet6 group: ff02::1:ffa3:cd6c
          inet6 group: ff02::1:ff00:2
          
Iface  5  HWaddr: 2E:82  Channel: 26  Page: 0  NID: 0x23  PHY: O-QPSK 
          
          Long HWaddr: 00:04:25:19:18:01:AE:82 
           TX-Power: 0dBm  State: IDLE  max. Retrans.: 3  CSMA Retries: 4 
          AUTOACK  ACK_REQ  CSMA  L2-PDU:102  MTU:1280  HL:64  RTR  
          RTR_ADV  6LO  IPHC  
          Source address length: 8
          Link type: wireless
          inet6 addr: fe80::204:2519:1801:ae82  scope: link  VAL
          inet6 addr: 2001:db8:0:2:204:2519:1801:ae82  scope: global  VAL
          inet6 group: ff02::2
          inet6 group: ff02::1
          inet6 group: ff02::1:ff01:ae82
          inet6 group: ff02::1a
          
> nib abr
nib abr
2001:db8:0:2:204:2519:1801:ae82 v0 expires 10000min
> rpl
rpl
instance table:	[X]	
parent table:	[ ]	[ ]	[ ]	

instance [0 | Iface: 5 | mop: 2 | ocp: 0 | mhri: 256 | mri 0]
	dodag [2001:db8:0:2:204:2519:1801:ae82 | R: 256 | OP: Router | PIO: on | TR(I=[8,20], k=10, c=0)]
> 6ctx
6ctx
cid|prefix                                     |C|ltime
-----------------------------------------------------------
  0|                         2001:db8:0:2::/64 |1|  666min

miri64 · 2021-07-29T13:51:00Z

Please squash and address the changes requested in #16672 (review)

benpicco requested review from PeterKietzmann, cgundogan, haukepetersen and miri64 as code owners July 22, 2021 14:28

github-actions bot added Area: network Area: Networking Area: sys Area: System labels Jul 22, 2021

benpicco force-pushed the gnrc_util_conf_prefix branch from 3b3c65d to bd15486 Compare July 22, 2021 15:01

benpicco added the Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation label Jul 22, 2021

benpicco requested a review from kaspar030 July 22, 2021 15:03

benpicco commented Jul 22, 2021

View reviewed changes

sys/include/net/gnrc/ipv6/util.h Outdated Show resolved Hide resolved

benpicco mentioned this pull request Jul 22, 2021

gnrc_ipv6_simple_subnets: auto-configuration for nested subnets on a simple tree topology #16536

Merged

miri64 requested changes Jul 22, 2021

View reviewed changes

benpicco commented Jul 23, 2021

View reviewed changes

miri64 requested changes Jul 23, 2021

View reviewed changes

benpicco force-pushed the gnrc_util_conf_prefix branch from bd15486 to ab3ed74 Compare July 27, 2021 22:31

miri64 reviewed Jul 28, 2021

View reviewed changes

benpicco force-pushed the gnrc_util_conf_prefix branch 2 times, most recently from ed50d40 to 078656c Compare July 28, 2021 16:46

miri64 reviewed Jul 29, 2021

View reviewed changes

sys/net/gnrc/application_layer/uhcpc/gnrc_uhcpc.c Outdated Show resolved Hide resolved

miri64 requested changes Jul 29, 2021

View reviewed changes

sys/net/gnrc/netif/gnrc_netif_util.c Outdated Show resolved Hide resolved

miri64 reviewed Jul 29, 2021

View reviewed changes

miri64 requested changes Jul 29, 2021

View reviewed changes

miri64 approved these changes Jul 29, 2021

View reviewed changes

benpicco force-pushed the gnrc_util_conf_prefix branch from a95c6e1 to c10a8e9 Compare July 29, 2021 13:52

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 29, 2021

benpicco changed the title ~~net/gnrc/ipv6: move gnrc_util_conf_prefix() to common code~~ gnrc_netif: add gnrc_netif_ipv6_add_prefix() & helper functions Jul 29, 2021

benpicco and others added 5 commits July 29, 2021 16:04

gnrc_netif: add gnrc_netif_ipv6_add_prefix()

b90c6ed

gnrc/rpl: allow const dodag ID

3dad587

gnrc/rpl: add gnrc_rpl_configure_root() convenience function

d999727

gnrc_dhcpv6_client: make use of gnrc_netif_ipv6_add_prefix()

a18f123

sys/net/uhcp: make use of gnrc_netif_ipv6_add_prefix()

6e3c09e

benpicco force-pushed the gnrc_util_conf_prefix branch from c10a8e9 to 6e3c09e Compare July 29, 2021 14:04

miri64 merged commit 6b5a181 into RIOT-OS:master Jul 29, 2021

benpicco deleted the gnrc_util_conf_prefix branch July 29, 2021 16:02

benpicco added this to the Release 2021.10 milestone Oct 20, 2021

	if (IS_USED(MODULE_GNRC_RPL)) {
	if (IS_USED(MODULE_GNRC_RPL) &&
	IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_6LBR)) {

Conversation

benpicco commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

via UHCP

Issues/PRs references

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!

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!

Uh oh!

benpicco commented Jul 23, 2021

Uh oh!

miri64 commented Jul 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 Jul 28, 2021 • 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!

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

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!

benpicco Jul 29, 2021 • 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!

miri64 Jul 29, 2021 • 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!

miri64 Jul 29, 2021 • 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!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

benpicco commented Jul 22, 2021 •

edited

Loading

miri64 Jul 28, 2021 •

edited

Loading

benpicco Jul 29, 2021 •

edited

Loading

miri64 Jul 29, 2021 •

edited

Loading

miri64 Jul 29, 2021 •

edited

Loading