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

Martine Lenders notifications at github.com
Tue Aug 10 07:09:21 CEST 2021


@miri64 requested changes on this pull request.

I think if this latest change requests are addressed, we are ready to merge.

> +    assert ia_na_addr.startswith(
+        ia_na_address_pool_prefix
+    ), "IA_NA address has not been assigned correctly"

Since you used an unusual prefix length (at least for RIOT right now), you could use Python's `ipaddress` module here to check if the prefix matches:

```py
assert IPv6_Address(ia_na_addr) in IPv6_Network("{}/80".format(ia_na_address_pool_prefix))
```

> +    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(
+        ia_na_address_pool_prefix
+    ), "IA_NA address has not been assigned correctly"
     child.expect(r"inet6 addr:\s+(?P<global_addr>[0-9a-f:]+)\s+scope: global")

Are you sure, it is always the case that the IA_NA assigned address always comes before the IA_PD generated one? If not it might be a better idea to first fetch both global addresses and then determine if one of them matches one condition and the other matches the other.

> +
+    for (int i = 0; i < CONFIG_GNRC_NETIF_IPV6_ADDRS_NUMOF; i++) {
+        if (ipv6_addr_equal(&netif->ipv6.addrs[i], addr)) {
+            netif->ipv6.addrs_flags[i] &= ~GNRC_NETIF_IPV6_ADDRS_FLAGS_STATE_MASK;
+            netif->ipv6.addrs_flags[i] |= GNRC_NETIF_IPV6_ADDRS_FLAGS_STATE_DEPRECATED;
+            return true;
+        }
+    }
+
+    return false;

I think here it makes sense to use `gnrc_netif_ipv6_addr_idx()`, as it is basically implementing the same loop you are using here (also note the usage of `gnrc_netif_acquire()`/`gnrc_netif_release()` so the deprectation operation is thread-safe):

```suggestion
    bool res = false;
    int i;
    
    gnrc_netif_acquire(netif);
    i = gnrc_netif_ipv6_addr_idx(netif, addr);
    if (i >= 0) {
        netif->ipv6.addrs_flags[i] &= ~GNRC_NETIF_IPV6_ADDRS_FLAGS_STATE_MASK;
        netif->ipv6.addrs_flags[i] |= GNRC_NETIF_IPV6_ADDRS_FLAGS_STATE_DEPRECATED;
        res = true;
    }
    gnrc_netif_release(netif);
    return res;
```

> +    for (int i = 0; i < CONFIG_GNRC_NETIF_IPV6_ADDRS_NUMOF; i++) {
+        if (ipv6_addr_equal(&netif->ipv6.addrs[i], addr)) {
+            gnrc_netif_ipv6_addr_remove(netif,
+                                        &netif->ipv6.addrs[i]);
+            return true;
+        }
+    }

This loop is not necessary, `gnrc_netif_ipv6_addr_remove()` will internally also loop over the addresses. As you do not check the return value of `dhcpv6_client_remove_addr()` I guess that one is also not needed:

```suggestion
    gnrc_netif_ipv6_addr_remove(netif, addr);
```

> + * @param[in] netif     Network interface the address was for.
+ * @param[in] addr      The assigned address.
+ *
+ * @return  true, if the address was configured successfully.
+ */
+bool dhcpv6_client_add_addr(unsigned netif, ipv6_addr_t *addr);
+
+/**
+ * @brief   Deprecates an existing address from an address lease.
+ *
+ * @param[in] netif     Network interface the address was for.
+ * @param[in] addr      The address to deprecate.
+ *
+ * @return  true, if the address was deprecated successfully.
+ */
+bool dhcpv6_client_deprecate_addr(unsigned netif, const ipv6_addr_t *addr);

Same here: You never check the return value, so is it really needed?

> + * @brief   Checks if the given network interface is configured
+ *          to use DHCPv6 IA_NA
+ *
+ * @param[in] netif     Network interface to check.
+ *
+ * @return  true, if the network interface is set up for IA_NA.
+ */
+bool dhcpv6_client_check_ia_na(unsigned netif);
+
+/**
+ * @brief   Configures a address lease that is provided by the server.
+ *
+ * @param[in] netif     Network interface the address was for.
+ * @param[in] addr      The assigned address.
+ *
+ * @return  true, if the address was configured successfully.

Why not keep the diagnostic error return code?

> +    int
+    prompt "Maximum number of unicast and anycast addresses per interface"

I think this change has the same effect as the original state

```suggestion
    int "Maximum number of unicast and anycast addresses per interface"
```

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


More information about the notifications mailing list