[riot-notifications] [RIOT-OS/RIOT] sys/net/dhcpv6: Add IA_NA support to the DHCPv6 client (#16228)

Martine Lenders notifications at github.com
Mon Aug 9 18:53:38 CEST 2021


@miri64 commented on this pull request.

Some preliminary review of the newest changes.

> @@ -185,6 +230,27 @@ void dhcpv6_client_req_ia_pd(unsigned netif, unsigned pfx_len)
     }
 }
 
+void dhcpv6_client_req_ia_na(unsigned netif)

Could make sense to return `-ENOMEM`, when there is no lease entry available anymore.

> +    if (!IS_USED(MODULE_DHCPV6_CLIENT_IA_NA)) {
+        return;
+    }
+
+    /* If no specific interface ID is given, check all
+       interfaces if DHCP IA_NA is enabled. Otherwise
+       use the specific interface ID. */
+    if (netif == SOCK_ADDR_ANY_NETIF) {
+        netif_t* netif = NULL;
+        while ((netif = netif_iter(netif))) {
+            int16_t netif_id = netif_get_id(netif);
+            if (netif_id < 0) {
+                continue;
+            }
+            if (dhcpv6_client_check_ia_na(netif_id)) {
+                dhcpv6_client_req_ia_na(netif_id);

... and then bail out here accordingly.

> +            gnrc_netif_ipv6_addr_remove_internal(netif,
+                                                 &netif->ipv6.addrs[i]);

You need to acquire the `netif` to use this function, however, I would rather elect to use the non-internal function. ;-)

> +
+        netif->ipv6.aac_mode |= GNRC_NETIF_AAC_DHCP;

Don't forget to clear the SLAAC flag, and that DHCPv6 may not be compiled in:

```suggestion
        if (IS_USED(MODULE_DHCPV6_CLIENT_IA_NA) {
            netif->ipv6.aac_mode &= ~GNRC_NETIF_AAC_SLAAC;
            netif->ipv6.aac_mode |= GNRC_NETIF_AAC_DHCP;
        }
        else {
            DEBUG("nib: would set interface %i  to DHCPv6, "
                  "but DHCPv6 is not provided", netif->pid);
        }
```

> @@ -15,6 +15,10 @@ def testfunc(child):
     child.expect(r"inet6 addr:\sfe80:[0-9a-f:]+\s+scope: link")
     child.expect(r"Iface\s+\d+")
     child.expect(r"inet6 addr:\s+fe80:[0-9a-f:]+\s+scope: link")
+    child.expect(r"inet6 addr:\s+(?P<ia_na_addr>[0-9a-f:]+)\s+scope: global")
+    ia_na_addr = child.match.group("ia_na_addr")
+    assert ia_na_addr.startswith(
+        "2001:db8:1::"), "IA_NA address has not been assigned correctly"

Maybe use a variable here instead of the literal `"2001:db8:1::"`?

> +                    dhcpv6_opt_iaaddr_t *this_iaaddr = (dhcpv6_opt_iaaddr_t *)ia_na_opt;
+                    if ((!lease->leased) ||
+                        (iaaddr == NULL)) {
+                        /* only take first address for now */
+                        iaaddr = this_iaaddr;
+                    }
+                    break;
+                }
+                default:
+                    break;
+            }
+        }
+        na_t1 = byteorder_ntohl(ia_na->t1);
+        na_t2 = byteorder_ntohl(ia_na->t2);
+        _update_t1_t2(na_t1, na_t2);
+        if ((iaaddr != NULL)) {

Superfluous parenthesis:

```suggestion
        if (iaaddr != NULL) {
```

> +        if ((iaaddr != NULL)) {
+            uint32_t valid = byteorder_ntohl(iaaddr->valid);
+            uint32_t pref = byteorder_ntohl(iaaddr->pref);
+
+            lease->leased = 1U;
+            memcpy(&lease->addr, &iaaddr->addr, sizeof(ipv6_addr_t));
+            if (dhcpv6_client_add_addr(lease->parent.ia_id.info.netif,
+                                        &lease->addr)) {
+                DEBUG("IP ADDRESS successfully added!\n");
+                lease->pref_until = pref;
+                lease->valid_until = valid;
+
+                _set_event_timeout_sec(&deprecate_timeout, &deprecate_addrs,
+                                        pref);
+                _set_event_timeout_sec(&remove_timeout, &remove_addrs,
+                                        valid);
+            }
+        }

Could be its own function IMHO

```C
void _update_addr_lease(const dhcpv6_opt_iaaddr_t *iaaddr, addr_lease_t *lease);
```

>  static pfx_lease_t pfx_leases[CONFIG_DHCPV6_CLIENT_PFX_LEASE_MAX];
 static server_t server;
-static event_timeout_t solicit_renew_timeout, rebind_timeout;
+static event_timeout_t solicit_renew_timeout, rebind_timeout,
+                       deprecate_timeout, remove_timeout;

I might be mistaken, but since deprecation timeout always happens before removal timeout, you might be able to safe some RAM here, by only using one timeout event for both event types.

> +
+        netif->ipv6.aac_mode |= GNRC_NETIF_AAC_DHCP;

Also, shouldn't `dhcpv6_client_req_ia_na()` be called here as well?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/16228#pullrequestreview-725587806
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210809/2b5285e4/attachment-0001.htm>


More information about the notifications mailing list