[riot-notifications] [RIOT-OS/RIOT] gnrc_ipv6_nib: auto-configure downstream subnets (#16536)

Martine Lenders notifications at github.com
Wed Aug 4 13:04:57 CEST 2021


@miri64 requested changes on this pull request.



> + *
+ * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @defgroup    net_gnrc_simple_subnets Simple-Subnet auto-configuration
+ * @ingroup     net_gnrc
+ * @brief       Automatic configuration for cascading subnets
+ *
+ * About
+ * =====
+ *
+ * This module provides an automatic configuration for networks with a simple
+ * tree tolpology.

```suggestion
 * tree topology.
```

> @@ -0,0 +1,40 @@
+ at startuml

Some inline doc on how to generate the SVG from this new (to RIOT) file format would be nice.

> + * the process repeats until the bits of prefix are exhausted. (The smallest
+ * must still have a /64 prefix.)

I know you mean "smallest subnet" here. Nevertheless, it is confusing that 64 is the largest possible number ;-).

> + * The downstream router will send an otherwise empty router advertisement with only
+ * the Route Information Option set to the upstream network.

Not sure the terminology "empty router advertisement" is correct. Maybe

```suggestion
 * The downstream router will send a router advertisement with only
 * a Route Information Option included to the upstream network.
```

> +    gnrc_pktsnip_t *ext_opts = NULL;
+    const ipv6_addr_t *prefix = &pio->prefix;
+    uint32_t valid_ltime = byteorder_ntohl(pio->valid_ltime);
+    uint32_t pref_ltime = byteorder_ntohl(pio->pref_ltime);
+
+    /* create a subnet for each downstream interface */
+    unsigned subnets = gnrc_netif_numof() - 1;
+
+    const uint8_t prefix_len = pio->prefix_len;
+    uint8_t new_prefix_len;
+
+    if (subnets == 0) {
+        return;
+    }
+
+    new_prefix_len = prefix_len + 32 - __builtin_clz(subnets);

Some comment what you are doing here would be nice. My head spins at least.

> +        /* create subnet by adding interface index */
+        new_prefix.u64[0].u64 = byteorder_ntohll(prefix->u64[0]);
+        new_prefix.u64[0].u64 |= (uint64_t)subnets-- << (63 - prefix_len);

??! Comment says something about interface index, but all I can see is the number of interfaces (which is called `subnets` here for some reason) and the prefix length.

> +    if (new_prefix_len > 64) {
+        DEBUG("simple_subnets: can't split /%u into %u subnets\n", prefix_len, subnets);
+        return;
+    }
+
+    while ((downstream = gnrc_netif_iter(downstream))) {
+        ipv6_addr_t new_prefix;
+
+        if (downstream == upstream) {
+            continue;
+        }
+
+        /* create subnet by adding interface index */
+        new_prefix.u64[0].u64 = byteorder_ntohll(prefix->u64[0]);
+        new_prefix.u64[0].u64 |= (uint64_t)subnets-- << (63 - prefix_len);
+        new_prefix.u64[0] = byteorder_htonll(new_prefix.u64[0].u64);

Doesn't this immediately overwrite the value from the two lines before?

> +        if (ext_opts == NULL) {
+            DEBUG("simple_subnets: No space left in packet buffer. Not adding RIO\n");

Does it make sense to send the RA in that case at all?

-- 
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/16536#pullrequestreview-722138400
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210804/ac987811/attachment.htm>


More information about the notifications mailing list