<p></p>
<p><b>@miri64</b> requested changes on this pull request.</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/16536#discussion_r699119028">sys/net/gnrc/routing/ipv6_auto_subnets/gnrc_ipv6_auto_subnets.c</a>:</p>
<pre style='color:#555'>> + * 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
</pre>
<p>What does "sufficiently large" mean in this context?</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/16536#discussion_r699119489">sys/net/gnrc/routing/ipv6_auto_subnets/gnrc_ipv6_auto_subnets.c</a>:</p>
<pre style='color:#555'>> + * There can only be a single routing node on each level of the network but an
+ * arbitrary number of leaf nodes.
</pre>
<p>I guess this needs to be fixed once <a class="issue-link js-issue-link" data-error-text="Failed to load title" data-id="972096196" data-permission-text="Title is private" data-url="https://github.com/RIOT-OS/RIOT/issues/16750" data-hovercard-type="pull_request" data-hovercard-url="/RIOT-OS/RIOT/pull/16750/hovercard" href="https://github.com/RIOT-OS/RIOT/pull/16750">#16750</a> is merged?</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/16536#discussion_r699134179">sys/net/gnrc/routing/ipv6_auto_subnets/gnrc_ipv6_auto_subnets.c</a>:</p>
<pre style='color:#555'>> + * 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
</pre>
<p>This is the first time the text mentions a "reduced prefix". This should be explained.</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/16536#discussion_r699134428">sys/net/gnrc/routing/ipv6_auto_subnets/gnrc_ipv6_auto_subnets.c</a>:</p>
<pre style='color:#555'>> + * the process repeats until the bits of prefix are exhausted. (The smallest subnet
+ * must still have a /64 prefix.)
</pre>

⬇️ Suggested change
<pre style="color: #555">- * the process repeats until the bits of prefix are exhausted. (The smallest subnet
- * must still have a /64 prefix.)
+ * the process repeats until the bits of prefix are exhausted. The smallest subnet
+ * must still have a /64 prefix.
</pre>


<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/16536#discussion_r699134563">sys/net/gnrc/routing/ipv6_auto_subnets/gnrc_ipv6_auto_subnets.c</a>:</p>
<pre style='color:#555'>> + * =====
+ *
+ * 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
</pre>

⬇️ Suggested change
<pre style="color: #555">- * the process repeats until the bits of prefix are exhausted. (The smallest subnet
+ * the process repeats until the bits of the prefix are exhausted. (The smallest subnet
</pre>


<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/16536#discussion_r699135089">sys/net/gnrc/routing/ipv6_auto_subnets/gnrc_ipv6_auto_subnets.c</a>:</p>
<pre style='color:#555'>> + * 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
</pre>
<p>Which "downstream router". From what I understood so far, there are many.</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/16536#discussion_r699137291">sys/net/gnrc/routing/ipv6_auto_subnets/gnrc_ipv6_auto_subnets.c</a>:</p>
<pre style='color:#555'>> + * 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
</pre>
<p>"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?</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/16536#discussion_r699139787">sys/net/gnrc/routing/ipv6_auto_subnets/gnrc_ipv6_auto_subnets.c</a>:</p>
<pre style='color:#555'>> + * 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.
</pre>
<p>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.</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/16536#discussion_r699140164">sys/net/gnrc/routing/ipv6_auto_subnets/gnrc_ipv6_auto_subnets.c</a>:</p>
<pre style='color:#555'>> + * ![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
</pre>
<p>How does one add a module to a node? Shouldn't it rather be added to the node's code ;-).</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/16536#discussion_r699143173">sys/net/gnrc/routing/ipv6_auto_subnets/gnrc_ipv6_auto_subnets.c</a>:</p>
<pre style='color:#555'>> +    /* 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;
+    }
</pre>
<p>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</p>

⬇️ Suggested change
<pre style="color: #555">-    /* 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;
-    }
+    memset(out, 0, sizeof(*out));
+    ipv6_addr_init_prefix(out, prefix, bits);
</pre>


<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/16536#discussion_r699144432">sys/net/gnrc/routing/ipv6_auto_subnets/gnrc_ipv6_auto_subnets.c</a>:</p>
<pre style='color:#555'>> +    /* 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
</pre>
<p>Not sure if it is wise to use unicode here (especially since it can easily expressed without:</p>

⬇️ Suggested change
<pre style="color: #555">-    /* 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
+    /* 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
</pre>


<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/16536#discussion_r699148907">sys/net/gnrc/routing/ipv6_auto_subnets/gnrc_ipv6_auto_subnets.c</a>:</p>
<pre style='color:#555'>> +
+        /* 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");
</pre>
<p>I might be mistaken, but I think, in case <code>ext_opts</code> was already set (as this is a loop this well may be and also with <code>_remove_old_prefix()</code>), you need to release <code>ext_opts</code> in this case. Otherwise there is a potential packet buffer leak here.</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/16536#discussion_r699145193">sys/net/gnrc/routing/ipv6_auto_subnets/gnrc_ipv6_auto_subnets.c</a>:</p>
<pre style='color:#555'>> +    /* 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
</pre>
<p>Besides having Unicode here in the first place: At first sight I also confused the flooring-brackets <code>⌊⌋</code> with normal brackets <code>[]</code> 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 <g-emoji class="g-emoji" alias="stuck_out_tongue_winking_eye" fallback-src="https://github.githubassets.com/images/icons/emoji/unicode/1f61c.png">😜</g-emoji>. I think among programers <code>floor()</code> in general is also better understood than <code>⌊⌋</code></p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/RIOT-OS/RIOT/pull/16536#pullrequestreview-742453745">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABE7WYB5BSVRE5RDDKAZVUDT7SON7ANCNFSM46IAKR5Q">unsubscribe</a>.<br />Triage notifications on the go with GitHub Mobile for <a href="https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675">iOS</a> or <a href="https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub">Android</a>.
<img src="https://github.com/notifications/beacon/ABE7WYBYIA5KHGXNCUUWHNDT7SON7A5CNFSM46IAKR52YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOFRAPD4I.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/RIOT-OS/RIOT/pull/16536#pullrequestreview-742453745",
"url": "https://github.com/RIOT-OS/RIOT/pull/16536#pullrequestreview-742453745",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>