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

Martine Lenders notifications at github.com
Wed Jul 21 10:56:53 CEST 2021


@miri64 commented on this pull request.

Sorry, only now saw your questions. I hope the review below gives some sufficient answers.

> @@ -111,7 +123,8 @@ extern "C" {
  * Default: 2 (1 link-local + 1 global address)

Maybe amend the doc here as well.

> @@ -111,7 +123,8 @@ extern "C" {
  * Default: 2 (1 link-local + 1 global address)
  */
 #ifndef CONFIG_GNRC_NETIF_IPV6_ADDRS_NUMOF
-#define CONFIG_GNRC_NETIF_IPV6_ADDRS_NUMOF    (2)
+#define CONFIG_GNRC_NETIF_IPV6_ADDRS_NUMOF    (2 + \
+                                               GNRC_NETIF_IPV6_DHCPV6_IA_NA_ADDR)

Then again (and sorry if I contradict past-me in that case, if so, please ignore): why have the indirection via `GNRC_NETIF_IPV6_DHCPV6_IA_NA_ADDR` in the first place. Just use

```suggestion
                                               CONFIG_DHCPV6_CLIENT_ADDR_LEASE_MAX)
```

here directly.

> @@ -22,12 +22,12 @@ config GNRC_NETIF_MSG_QUEUE_SIZE_EXP
 
 config GNRC_NETIF_IPV6_ADDRS_NUMOF
     int "Maximum number of unicast and anycast addresses per interface"
-    default 2
+    default 3

@leandrolanzieri, is something like

```suggestion
    default 2 + CONFIG_DHCPV6_CLIENT_ADDR_LEASE_MAX if CONFIG_DHCPV6_CLIENT_ADDR_LEASE_MAX
    default 2
```

possible?

> -    default 2
+    default 3
     help
         If you change this, please make sure that
         @ref CONFIG_GNRC_NETIF_IPV6_GROUPS_NUMOF is also large enough to fit the
         addresses' solicited nodes multicast addresses.
-        Default: 2 (1 link-local + 1 global address).
+        Default: 3 (1 link-local + 2 global addresses).

If you use `CONFIG_DHCPV6_CLIENT_ADDR_LEASE_MAX` to define the maximum number of addresses instead I think you can use that to define a conditional default.

> @@ -101,6 +101,18 @@ extern "C" {
 #define GNRC_NETIF_IPV6_RTR_ADDR   (0)
 #endif
 
+/**
+ * @brief   Number of global unicast addresses needed for a
+ *          @ref net_dhcpv6_client "DHCPv6 Client" using IA_NA
+ *
+ * @note    Used for calculation of @ref CONFIG_GNRC_NETIF_IPV6_ADDRS_NUMOF
+ */
+#ifdef MODULE_GNRC_DHCPV6_CLIENT_IA_NA
+#define GNRC_NETIF_IPV6_DHCPV6_IA_NA_ADDR   (1)

Yes. And for brevity maybe use

```suggestion
#define GNRC_NETIF_IPV6_DHCPV6_IA_NA_NUMOF  (1)
```

instead. The NA stands for non-temporary _address_ after all, so the `ADDR` is somewhat redundant ;-).

> @@ -59,6 +59,13 @@ extern "C" {
 #define CONFIG_DHCPV6_CLIENT_PFX_LEASE_MAX (1U)
 #endif
 
+/**
+ * @brief   Maximum number of leases to be stored
+ */
+#ifndef CONFIG_DHCPV6_CLIENT_ADDR_LEASE_MAX
+#define CONFIG_DHCPV6_CLIENT_ADDR_LEASE_MAX (1U)

I'd say vice versa. The number of non-temporary addresses is after all dependent on the number of leases you can have, not the other way around.

> +                        char addr_str[IPV6_ADDR_MAX_STR_LEN];
+
+                        if (&lease->addr != NULL &&
+                            !ipv6_addr_equal(&lease->addr, addr)) {
+                            /* A different address has been leased to the client */
+                            netif_set_opt(netif, NETOPT_IPV6_ADDR_REMOVE, 64 << 8, &lease->addr, sizeof(lease->addr));
+                        }
+
+                        lease->leased = 1U;
+                        memcpy(&lease->addr, addr, sizeof(ipv6_addr_t));
+                        DEBUG("DHCPv6 client: ADD IP ADDRESS %s\n",
+                            ipv6_addr_to_str(addr_str, addr, sizeof(addr_str)));
+
+                        if (netif_set_opt(netif, NETOPT_IPV6_ADDR, 64 << 8, addr, sizeof(*addr)) > 0) {

Yes. See https://github.com/RIOT-OS/RIOT/pull/16228#issuecomment-873427693.

-- 
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-711409729
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210721/d9893d3e/attachment-0001.htm>


More information about the notifications mailing list