[riot-notifications] [RIOT-OS/RIOT] gnrc_ipv6_simple_subnets: auto-configuration for nested subnets on a simple tree topology (#16536)

Martine Lenders notifications at github.com
Tue Aug 31 11:31:11 CEST 2021


@miri64 requested changes on this pull request.



> + * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @defgroup    net_gnrc_ipv6_auto_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 topology.
+ *
+ * If a sufficiently large IPv6 prefix is provided via Router Advertisements, a

What does "sufficiently large" mean in this context?

> + * There can only be a single routing node on each level of the network but an
+ * arbitrary number of leaf nodes.

I guess this needs to be fixed once #16750 is merged?

> + * About
+ * =====
+ *
+ * This module provides an automatic configuration for networks with a simple
+ * tree topology.
+ *
+ * If a sufficiently large IPv6 prefix is provided via Router Advertisements, a
+ * routing node with this module will automatically configure subnets from it for
+ * each downstream interface.
+ *
+ * There can only be a single routing node on each level of the network but an
+ * arbitrary number of leaf nodes.
+ *
+ * ![Example Topology](gnrc_ipv6_auto_subnets_simple.svg)
+ *
+ * The downstream network(s) get the reduced prefix via Router Advertisements and

This is the first time the text mentions a "reduced prefix". This should be explained.

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

```suggestion
 * the process repeats until the bits of prefix are exhausted. The smallest subnet
 * must still have a /64 prefix.
```

> + * =====
+ *
+ * This module provides an automatic configuration for networks with a simple
+ * tree topology.
+ *
+ * If a sufficiently large IPv6 prefix is provided via Router Advertisements, a
+ * routing node with this module will automatically configure subnets from it for
+ * each downstream interface.
+ *
+ * There can only be a single routing node on each level of the network but an
+ * arbitrary number of leaf nodes.
+ *
+ * ![Example Topology](gnrc_ipv6_auto_subnets_simple.svg)
+ *
+ * The downstream network(s) get the reduced prefix via Router Advertisements and
+ * the process repeats until the bits of prefix are exhausted. (The smallest subnet

```suggestion
 * the process repeats until the bits of the prefix are exhausted. (The smallest subnet
```

> + * tree topology.
+ *
+ * If a sufficiently large IPv6 prefix is provided via Router Advertisements, a
+ * routing node with this module will automatically configure subnets from it for
+ * each downstream interface.
+ *
+ * There can only be a single routing node on each level of the network but an
+ * arbitrary number of leaf nodes.
+ *
+ * ![Example Topology](gnrc_ipv6_auto_subnets_simple.svg)
+ *
+ * The downstream network(s) get the reduced prefix via Router Advertisements and
+ * the process repeats until the bits of prefix are exhausted. (The smallest subnet
+ * must still have a /64 prefix.)
+ *
+ * The downstream router will send a router advertisement with only a

Which "downstream router". From what I understood so far, there are many.

> + * routing node with this module will automatically configure subnets from it for
+ * each downstream interface.
+ *
+ * There can only be a single routing node on each level of the network but an
+ * arbitrary number of leaf nodes.
+ *
+ * ![Example Topology](gnrc_ipv6_auto_subnets_simple.svg)
+ *
+ * The downstream network(s) get the reduced prefix via Router Advertisements and
+ * the process repeats until the bits of prefix are exhausted. (The smallest subnet
+ * must still have a /64 prefix.)
+ *
+ * The downstream router will send a router advertisement with only a
+ * Route Information Option included to the upstream network.
+ * The Route Information Option contains the prefix of the downstream network so that
+ * upstream hosts will no longer consider hosts in this subnet on-link but instead

"upstream hosts"... Why are hosts (traditionally used to describe non-routing nodes) suddenly involved with routing. Given that they are upstream, shouldn't they be routers?

> + * The Route Information Option contains the prefix of the downstream network so that
+ * upstream hosts will no longer consider hosts in this subnet on-link but instead
+ * will use the downstream router to route to the new subnet.

Why is this sentence important? Up until now, the text describes the routing protocol. This sentence suddenly makes a hard break, and throws around the stirring wheel into NDP. This should be carried over better.

> + * ![Example Topology](gnrc_ipv6_auto_subnets_simple.svg)
+ *
+ * The downstream network(s) get the reduced prefix via Router Advertisements and
+ * the process repeats until the bits of prefix are exhausted. (The smallest subnet
+ * must still have a /64 prefix.)
+ *
+ * The downstream router will send a router advertisement with only a
+ * Route Information Option included to the upstream network.
+ * The Route Information Option contains the prefix of the downstream network so that
+ * upstream hosts will no longer consider hosts in this subnet on-link but instead
+ * will use the downstream router to route to the new subnet.
+ *
+ * Usage
+ * =====
+ *
+ * Simply add the `gnrc_ipv6_auto_subnets` module to the nodes that should act as routers

How does one add a module to a node? Shouldn't it rather be added to the node's code ;-).

> +    /* first copy old prefix */
+    memcpy(out, prefix, bytes);
+
+    /* make sure unused bits are cleared */
+    if (rem) {
+        uint8_t mask = 0xff << (8 - rem);
+        out->u8[bytes] = prefix->u8[bytes] & mask;
+    } else {
+        out->u8[bytes] = 0;
+    }

Unless you are under timing constraints, I think this can be solved easier to understand (and I think also with smaller code footprint, as the functions is already used elsewhere in GNRC), with

```suggestion
    memset(out, 0, sizeof(*out));
    ipv6_addr_init_prefix(out, prefix, bits);
```



> +    /* Calculate remaining prefix length. For n subnets we consume ⌊log₂ n⌋ + 1 bits.
+     * To calculate ⌊log₂ n⌋ quickly, find the position of the most significant set bit

Not sure if it is wise to use unicode here (especially since it can easily expressed without:

```suggestion
    /* Calculate remaining prefix length. For n subnets we consume floor(log_2 n) + 1 bits.
     * To calculate floor(log_2 n) quickly, find the position of the most significant set bit
```

> +
+        /* first remove old prefix if the prefix changed */
+        _remove_old_prefix(downstream, &new_prefix, new_prefix_len, &ext_opts);
+
+        /* configure subnet on downstream interface */
+        gnrc_netif_ipv6_add_prefix(downstream, &new_prefix, new_prefix_len,
+                                   valid_ltime, pref_ltime);
+
+        /* start advertising subnet */
+        gnrc_ipv6_nib_change_rtr_adv_iface(downstream, true);
+
+        /* add route information option with new subnet */
+        tmp = gnrc_ndp_opt_ri_build(&new_prefix, new_prefix_len, valid_ltime,
+                                    NDP_OPT_RI_FLAGS_PRF_NONE, ext_opts);
+        if (tmp == NULL) {
+            DEBUG("auto_subnets: No space left in packet buffer. Not adding RIO\n");

I might be mistaken, but I think, in case `ext_opts` was already set (as this is a loop this well may be and also with `_remove_old_prefix()`), you need to release `ext_opts` in this case. Otherwise there is a potential packet buffer leak here.

> +    /* Calculate remaining prefix length. For n subnets we consume ⌊log₂ n⌋ + 1 bits.
+     * To calculate ⌊log₂ n⌋ quickly, find the position of the most significant set bit

Besides having Unicode here in the first place: At first sight I also confused the flooring-brackets `⌊⌋` with normal brackets `[]` because I have a very small font-size config. Then I thought something is wrong with the display and only then I realized you where using Unicode here :stuck_out_tongue_winking_eye:. I think among programers `floor()` in general is also better understood than `⌊⌋`

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


More information about the notifications mailing list