Changeset 3e6bca8 in mainline

Timestamp:

2021-08-08T17:30:29Z (3 years ago)

Author:

Jiri Svoboda <jiri@…>

Branches:

master, serial, ticket/834-toolchain-update, topic/msim-upgrade, topic/simplify-dev-export

Children:

a7f7b9c3

Parents:

b4edc96

Message:

Represent Ethernet address as a number instead of an array

Carefully document the design since this breaks the principle of least
surprise. Also add unit tests.

Location:

uspace

Files:

• app/inet/inet.c (modified) (3 diffs)
• lib/drv/generic/remote_ieee80211.c (modified) (7 diffs)
• lib/inet/include/inet/eth_addr.h (modified) (2 diffs)
• lib/inet/meson.build (modified) (1 diff)
• lib/inet/src/addr.c (modified) (2 diffs)
• lib/inet/src/eth_addr.c (modified) (3 diffs)
• lib/inet/test/eth_addr.c (added)
• lib/inet/test/main.c (added)
• srv/net/dhcp/dhcp.c (modified) (2 diffs)
• srv/net/inetsrv/inet_link.c (modified) (2 diffs)

uspace/app/inet/inet.c

@@ -37 +37 @@
 #include <errno.h>
 #include <inet/addr.h>
+#include <inet/eth_addr.h>
 #include <inet/inetcfg.h>
 #include <io/table.h>
@@ -340 +341 @@
         inet_link_info_t linfo;
         table_t *table = NULL;
+        eth_addr_str_t saddr;
 
         size_t count;
@@ -368 +370 @@
                 }
 
-                table_printf(table, "%02x:%02x:%02x:%02x:%02x:%02x\t"
-                    "%s\t" "%zu\n",
-                    linfo.mac_addr.b[0], linfo.mac_addr.b[1],
-                    linfo.mac_addr.b[2], linfo.mac_addr.b[3],
-                    linfo.mac_addr.b[4], linfo.mac_addr.b[5],
-                    linfo.name, linfo.def_mtu);
+                eth_addr_format(&linfo.mac_addr, &saddr);
+                table_printf(table, "%s\t %s\t %zu\n", saddr.str, linfo.name,
+                    linfo.def_mtu);
 
                 free(linfo.name);

uspace/lib/drv/generic/remote_ieee80211.c

@@ -1 +1 @@
 /*
+ * Copyright (c) 2021 Jiri Svoboda
  * Copyright (c) 2015 Jan Kolarik
  * All rights reserved.
@@ -84 +85 @@
 }
 
-static bool mac_matches(uint8_t *mac1, uint8_t *mac2)
-{
-        for (size_t i = 0; i < ETH_ADDR; i++) {
-                if (mac1[i] != mac2[i])
-                        return false;
-        }
-
-        return true;
-}
-
+// XXX This is wrong. Wifi should not have anything to do with IP links
 static sysarg_t get_link_id(uint8_t *mac)
 {
         sysarg_t *link_list;
         inet_link_info_t link_info;
+        eth_addr_t eth_addr;
         size_t count;
+
+        eth_addr_decode(mac, &eth_addr);
 
         errno_t rc = inetcfg_get_link_list(&link_list, &count);
@@ -109 +104 @@
                         return -1;
 
-                if (mac_matches(mac, link_info.mac_addr.b))
+                if (eth_addr_compare(&eth_addr, &link_info.mac_addr) == 0)
                         return link_list[i];
         }
@@ -170 +165 @@
                 return rc;
 
+        // XXX This is wrong. Wifi should not initiate DHCP
+
         /* Send DHCP discover. */
         nic_address_t wifi_mac;
@@ -207 +204 @@
         if (rc != EOK)
                 return rc;
+
+        eth_addr_t eth_addr;
+        eth_addr_decode(wifi_mac.address, &eth_addr);
 
         inet_link_info_t link_info;
@@ -215 +215 @@
         size_t count;
 
+        /// XXX This is wrong. Wifi should do nothing with DHCP
+
         /* Remove previous DHCP address. */
         rc = inetcfg_get_addr_list(&addr_list, &count);
@@ -229 +231 @@
                         return rc;
 
-                if (mac_matches(wifi_mac.address, link_info.mac_addr.b)) {
+                if (eth_addr_compare(&eth_addr, &link_info.mac_addr) == 0) {
                         if (str_test_prefix(addr_info.name, "dhcp")) {
                                 rc = inetcfg_addr_delete(addr_list[i]);

uspace/lib/inet/include/inet/eth_addr.h

@@ -41 +41 @@
 
 #define ETH_ADDR_SIZE 6
+#define ETH_ADDR_STR_SIZE (6 * 2 + 5)
 
+#define ETH_ADDR_INITIALIZER(aa, bb, cc, dd, ee, ff) \
+    { .a = ((uint64_t)(aa) << 40) | ((uint64_t)(bb) << 32) | \
+    ((uint64_t)(cc) << 24) | ((uint64_t)(dd) << 16) | \
+    ((uint64_t)(ee) << 8) | (ff) }
+
+/** Ethernet address.
+ *
+ * Defined as a structure. This provides strong type checking.
+ *
+ * Since the structure is not opaque, this allows eth_addr_t to be
+ * allocated statically and copied around using the assignment operator.
+ *
+ * It is stored in the lower 48 bits of a 64-bit integer. This is an internal
+ * representation that allows simple and efficient operation. Most CPUs
+ * will be much faster (and we will need less instructions) operating
+ * on a single 64-bit integer than on six individual 8-bit integers.
+ *
+ * Kind reader will appreciate the cleverness and elegance of this
+ * representation.
+ */
 typedef struct {
-        uint8_t b[ETH_ADDR_SIZE];
+        uint64_t a;
 } eth_addr_t;
+
+/** Ethernet address in the form of a string */
+typedef struct {
+        char str[ETH_ADDR_STR_SIZE + 1];
+} eth_addr_str_t;
 
 extern const eth_addr_t eth_addr_broadcast;
@@ -52 +78 @@
 
 extern int eth_addr_compare(const eth_addr_t *, const eth_addr_t *);
+extern void eth_addr_format(eth_addr_t *, eth_addr_str_t *);
 
 #endif

uspace/lib/inet/meson.build

@@ -44 +44 @@
         'src/udp.c',
 )
+
+test_src = files(
+        'test/eth_addr.c',
+        'test/main.c',
+)

uspace/lib/inet/src/addr.c

@@ -55 +55 @@
 const addr32_t addr32_broadcast_all_hosts = 0xffffffff;
 
-static const eth_addr_t inet_eth_addr_solicited_node = {
-        0x33, 0x33, 0xff, 0, 0, 0
-};
+static eth_addr_t inet_eth_addr_solicited_node =
+    ETH_ADDR_INITIALIZER(0x33, 0x33, 0xff, 0, 0, 0);
 
 static const inet_addr_t inet_addr_any_addr = {
@@ -91 +90 @@
 void eth_addr_solicited_node(const addr128_t ip, eth_addr_t *mac)
 {
-        memcpy(&mac->b[0], &inet_eth_addr_solicited_node.b[0], 3);
-        memcpy(&mac->b[3], ip + 13, 3);
+        uint8_t b[6];
+        mac->a = inet_eth_addr_solicited_node.a;
+
+        eth_addr_encode(&inet_eth_addr_solicited_node, b);
+        memcpy(&b[3], ip + 13, 3);
+        eth_addr_decode(b, mac);
 }
 

uspace/lib/inet/src/eth_addr.c

@@ -37 +37 @@
 #include <inet/eth_addr.h>
 #include <mem.h>
+#include <stdio.h>
 
-const eth_addr_t eth_addr_broadcast = {
-        0xff, 0xff, 0xff, 0xff, 0xff, 0xff
-};
+const eth_addr_t eth_addr_broadcast =
+    ETH_ADDR_INITIALIZER(0xff, 0xff, 0xff, 0xff, 0xff, 0xff);
 
 void eth_addr_encode(eth_addr_t *addr, void *buf)
 {
         uint8_t *bp = (uint8_t *)buf;
+        uint64_t a;
+        int i;
 
-        memcpy(bp, &addr->b[0], ETH_ADDR_SIZE);
+        a = addr->a;
+
+        for (i = 0; i < ETH_ADDR_SIZE; i++)
+                bp[i] = (a >> (40 - 8 * i)) & 0xff;
 }
 
@@ -52 +57 @@
 {
         const uint8_t *bp = (uint8_t *)buf;
+        uint64_t a;
+        int i;
 
-        memcpy(&addr->b[0], bp, ETH_ADDR_SIZE);
+        a = 0;
+        for (i = 0; i < ETH_ADDR_SIZE; i++)
+                a |= (uint64_t)bp[i] << (40 - 8 * i);
+
+        addr->a = a;
 }
 
@@ -62 +73 @@
 int eth_addr_compare(const eth_addr_t *a, const eth_addr_t *b)
 {
-        return memcmp(a->b, b->b, ETH_ADDR_SIZE) == 0;
+        if (a->a < b->a)
+                return -1;
+        else if (a->a == b->a)
+                return 0;
+        else
+                return 1;
+}
+
+void eth_addr_format(eth_addr_t *addr, eth_addr_str_t *saddr)
+{
+        int i;
+
+        snprintf(saddr->str, 3, "%02x",
+            (unsigned)((addr->a >> 40) & 0xff));
+        for (i = 1; i < ETH_ADDR_SIZE; i++) {
+                snprintf(saddr->str + 2 + 3 * (i - 1), 4, ":%02x",
+                    (unsigned)((addr->a >> (40 - i * 8)) & 0xff));
+        }
 }
 

uspace/srv/net/dhcp/dhcp.c

@@ -162 +162 @@
         hdr->flags = flag_broadcast;
 
-        memcpy(dlink->link_info.mac_addr.b, hdr->chaddr,
-            sizeof(dlink->link_info.mac_addr.b));
+        eth_addr_encode(&dlink->link_info.mac_addr, hdr->chaddr);
         hdr->opt_magic = host2uint32_t_be(dhcp_opt_magic);
 
@@ -187 +186 @@
         hdr->flags = flag_broadcast;
         hdr->ciaddr = host2uint32_t_be(offer->oaddr.addr);
-        memcpy(hdr->chaddr, dlink->link_info.mac_addr.b, 6);
+        eth_addr_encode(&dlink->link_info.mac_addr, hdr->chaddr);
         hdr->opt_magic = host2uint32_t_be(dhcp_opt_magic);
 

uspace/srv/net/inetsrv/inet_link.c

@@ -74 +74 @@
     addr128_t ip_addr)
 {
+        uint8_t b[ETH_ADDR_SIZE];
+
         memcpy(ip_addr, link_local_node_ip, 16);
-
-        ip_addr[8] = mac_addr->b[0] ^ 0x02;
-        ip_addr[9] = mac_addr->b[1];
-        ip_addr[10] = mac_addr->b[2];
-        ip_addr[13] = mac_addr->b[3];
-        ip_addr[14] = mac_addr->b[4];
-        ip_addr[15] = mac_addr->b[5];
+        eth_addr_encode(mac_addr, b);
+
+        ip_addr[8] = b[0] ^ 0x02;
+        ip_addr[9] = b[1];
+        ip_addr[10] = b[2];
+        ip_addr[13] = b[3];
+        ip_addr[14] = b[4];
+        ip_addr[15] = b[5];
 }
 
@@ -124 +127 @@
 static errno_t inet_iplink_change_addr(iplink_t *iplink, eth_addr_t *mac)
 {
+        eth_addr_str_t saddr;
+
+        eth_addr_format(mac, &saddr);
         log_msg(LOG_DEFAULT, LVL_DEBUG, "inet_iplink_change_addr(): "
-            "new addr=%02x:%02x:%02x:%02x:%02x:%02x",
-            mac->b[0], mac->b[1], mac->b[2], mac->b[3], mac->b[4], mac->b[5]);
+            "new addr=%s", saddr.str);
 
         list_foreach(inet_links, link_list, inet_link_t, ilink) {