[riot-notifications] [RIOT-OS/RIOT] sys/net/dhcpv6: Refactor IA_NA implementation (#16724)

Martine Lenders notifications at github.com
Thu Aug 12 00:10:20 CEST 2021


@miri64 requested changes on this pull request.



>      """Check if the expected IA_NA address has been assigned"""
-    return IPv6Address(ia_na_addr) in IPv6Network("{}/64".format(IA_NA_ADDRESS_POOL_PREFIX))
+    result = IPv6Address(ia_na_addr) in IPv6Network(f"{IA_NA_ADDRESS_POOL_PREFIX}/64")
+    result &= check_prefix(ia_na_addr, global_pfx)

Are you sure you want to use bit-wise AND here?

> -    """Perform IA_NA check for the first and IA_PD for the second address"""
-    return {
-        "ia_na_check": check_ia_na_addr(ia_na_addr),
-        "ia_pd_check": check_ia_pd_addr(ia_pd_addr, global_pfx),
-    }
-
-
-def test_global_addrs(global_addr_1, global_addr_2, global_pfx):
-    """Assert that one global address is the IA_NA and the other one is the IA_PD address"""
-    result_1 = check_global_addrs(global_addr_1, global_addr_2, global_pfx)
-    result_2 = check_global_addrs(global_addr_2, global_addr_1, global_pfx)
-    assert result_1 != result_2
-    assert result_1["ia_na_check"] != result_2["ia_na_check"]
-    assert result_1["ia_pd_check"] != result_2["ia_pd_check"]
+    result = IPv6Address(ia_pd_addr) in IPv6Network(f"{IA_PD_PREFIX}/33")
+    result &= check_prefix(ia_pd_addr, global_pfx)

Are you sure you want to use bit-wise AND here?

> @@ -15,6 +15,11 @@
 
 
 IA_NA_ADDRESS_POOL_PREFIX = "2001:db8:1::"
+IA_PD_PREFIX = "2001:db8:8000::"
+
+
+def assert_unique_elements(*args):
+    assert len(args) == len(set(args))

This does not need to be a function and makes it unnecessarily harder to trace the actual spot the error happened. Rather just say e.g.

```py
assert global_addr_1 != global_addr_2
```

below.

> +    global_addr_1 = extract_global_address(child)
+    global_addr_2 = extract_global_address(child)
+    assert_unique_elements(global_addr_1, global_addr_2)
+
+    global_pfx_1 = extract_global_prefix(child)
+    global_pfx_2 = extract_global_prefix(child)
+    assert_unique_elements(global_pfx_1, global_pfx_2)
+
+    test_data = [(global_addr_1, global_pfx_1), (global_addr_2, global_pfx_2)]

I would prefer a list of dictionaries here for better maintainability.

> +    result = [
+        (addr, prefix) for addr, prefix in test_data if test_function(addr, prefix)
+    ]
+    assert len(result) == 1

Someone likes to flex there list comprehension game? I find this hard to understand without any comments.

-- 
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/16724#pullrequestreview-727994117
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210811/db472a89/attachment.htm>


More information about the notifications mailing list