[riot-notifications] [RIOT-OS/RIOT] gnrc/ipv6_auto_subnets: relax topology requirements (#16750)

Martine Lenders notifications at github.com
Mon Sep 27 22:29:06 CEST 2021


@miri64 requested changes on this pull request.

> @miri64 do you think we can still get this into the release?

Ok, let's try this.

Some function names are IMHO confusing. Yes you documented the functions, but if you just scroll through the code, see a function `_insert` or `_compare_addr` it is not immediately clear what is happening.

> +        DEBUG("auto_subnets: unable to allocate UDP header\n");
+        gnrc_pktbuf_release(payload);
+        return -ENOBUFS;
+    }
+
+    /* allocate IPv6 header */
+    ip = gnrc_ipv6_hdr_build(udp, NULL, addr);
+    if (ip == NULL) {
+        DEBUG("auto_subnets: unable to allocate IPv6 header\n");
+        gnrc_pktbuf_release(udp);
+        return -ENOBUFS;
+    }
+
+    /* add netif header, if interface was given */
+    if (netif != NULL) {
+        gnrc_pktsnip_t *netif_hdr = gnrc_netif_hdr_build(NULL, 0, NULL, 0);

Shouldn't there be a check if this is `NULL` as well?

```suggestion
        gnrc_pktsnip_t *netif_hdr = gnrc_netif_hdr_build(NULL, 0, NULL, 0);
        
        if (netif_hdr == NULL) {
            DEBUG("auto_subnets: unable to allocate netif header\n");
            gnrc_pktbuf_release(ip);
            return -ENOBUFS;
        }
```

> +    };
+
+    msg_send(&m, _server_pid);
+#endif /* !IS_USED(MODULE_GNRC_IPV6_AUTO_SUBNETS_SIMPLE) */
+}
+
+#if !IS_USED(MODULE_GNRC_IPV6_AUTO_SUBNETS_SIMPLE)
+/**
+ * @brief Check if memory region is set to 0
+ *
+ * @param[in]   The memory array to check
+ * @param[in]   The size of the memory array
+ *
+ * @return  true if all bytes are set to 0
+ */
+static bool _is_empty(const uint8_t *addr, size_t len)

What `_is_empty`?

```suggestion
static bool _array_is_empty(const uint8_t *addr, size_t len)
```

e.g. would be better.

> +        }
+    }
+    return true;
+}
+
+/**
+ * @brief Insert a l2 address into the `l2addrs` array
+ *
+ * @param[in]   addr    The l2 address to insert
+ * @param[in]   len     l2 address length
+ *
+ * @return  1 if the address was added to the `l2addrs` array
+ *          0 if the address was already in the array
+ *         -1 if there was no more space in the `l2addrs` array
+ */
+static int _insert(const void *addr, size_t len)

What is `_insert`ed where?

```suggestion
static int _alloc_l2addr_entry(const void *addr, size_t len)
```

would be better IMHO

> + * @brief Insert a l2 address into the `l2addrs` array
+ *
+ * @param[in]   addr    The l2 address to insert
+ * @param[in]   len     l2 address length
+ *
+ * @return  1 if the address was added to the `l2addrs` array
+ *          0 if the address was already in the array
+ *         -1 if there was no more space in the `l2addrs` array

"Insert" might need to be replaced with "Allocates ... in" here for the doc to be aligned with that name chance.

> +/**
+ * @brief Compare the l2 address of the received packet with the l2 address of the
+ *        interface it was received on.
+ *
+ *        Only the first packet from a host generates a comparison, all subsequent
+ *        packets will be ignored until the `l2addrs` array is reset.
+ *
+ * @param[in] upstream interface, ignore if the source does not match
+ * @param[in] pkt   a received packet
+ *
+ * @return  1 if the sender l2 address is in order before the local l2 address
+ * @return  0 if the order could not be determined or a packet from the sender
+ *            was already processed
+ * @return -1 if the sender l2 address is in order behind the local l2 address
+ */
+static int _compare_addr(gnrc_netif_t *iface, gnrc_pktsnip_t *pkt)

Given that none the parameters is an address, the name of the function, `_compare_addr`, is somewhat confusing... Not sure about a better name, though `_get_my_l2addr_rank`, maybe?

-- 
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/16750#pullrequestreview-764711573
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210927/9f762917/attachment-0001.htm>


More information about the notifications mailing list