datastorage: improve linked list

- Simplify linked list search - code was unnecessarily complex for no benefit
- Adjust some MAC address handling to simplify code

[cleanup commit message]
Signed-off-by: Nick Hainke <vincent@systemli.org>

src/include/datastorage.h

@@ -25,11 +25,11 @@ struct mac_entry_s {
 // ---------------- Functions ----------
 void insert_macs_from_file();
 
-int insert_to_maclist(struct dawn_mac mac);
+struct mac_entry_s* insert_to_maclist(struct dawn_mac mac);
 
-int mac_in_maclist(struct dawn_mac mac);
+struct mac_entry_s* mac_find_entry(struct dawn_mac mac);
 
-struct mac_entry_s* insert_to_mac_array(struct mac_entry_s* entry, struct mac_entry_s** insert_pos);
+struct mac_entry_s* insert_to_mac_array(struct mac_entry_s* entry);
 
 void mac_array_delete(struct mac_entry_s* entry);
 

src/include/mac_utils.h

@@ -23,6 +23,9 @@ struct __attribute__((__packed__)) dawn_mac
         uint8_t u8[ETH_ALEN];
 };
 
+// Used as a filler where a value is required but not used functionally
+extern const struct dawn_mac dawn_mac_null;
+
 // Compare a raw MAC address to 00:00:00:00:00:00
 #define mac_is_null(a1) ((a1)[0] == 0) && ((a1)[1] == 0) && ((a1)[2] == 0) && ((a1)[3] == 0) && ((a1)[4] == 0) && ((a1)[5] == 0)
 
@@ -41,6 +44,12 @@ struct __attribute__((__packed__)) dawn_mac
  */
 int hwaddr_aton(const char* txt, uint8_t* addr);
 
+/**
+ * Parse MAC from string.
+ * @param s
+ */
+struct dawn_mac str2mac(char* s);
+
 /**
  * Write mac to a file.
  * @param path

src/storage/datastorage.c

@@ -68,9 +68,6 @@ int mac_set_last = 0;
 // TODO: No longer used in code: retained to not break message xfer, etc
 char sort_string[SORT_LENGTH];
 
-// Used as a filler where a value is required but not used functionally
-static const struct dawn_mac dawn_mac_null = { .u8 = {0,0,0,0,0,0} };
-
 /*
 ** The ..._find_first() functions perform an efficient search of the core storage linked lists.
 ** "Skipping" linear searches and binary searches are used depending on anticipated array size.
@@ -325,42 +322,18 @@ static client** client_find_first_c_entry(struct dawn_mac client_mac)
 }
 #endif
 
-
+struct mac_entry_s* mac_find_entry(struct dawn_mac mac)
-static struct mac_entry_s** mac_find_first_entry(struct dawn_mac mac)
 {
-    int lo = 0;
-    struct mac_entry_s** lo_ptr = &mac_set;
-    int hi = mac_set_last;
-
     dawnlog_debug_func("Entering...");
 
-    while (lo < hi) {
+    struct mac_entry_s* ret = mac_set;
-        struct mac_entry_s** i = lo_ptr;
-        int scan_pos = lo;
 
-        // m is next test position of binary search
+    while (ret && mac_compare_bb(ret->mac, mac) != 0)
-        int m = (lo + hi) / 2;
+    {
-
+        ret = ret->next_mac;
-        // find entry with ordinal position m
-        while (scan_pos++ < m)
-        {
-            i = &((*i)->next_mac);
-        }
-
-        int this_cmp = mac_compare_bb((*i)->mac, mac);
-
-        if (this_cmp < 0)
-        {
-            lo = m + 1;
-            lo_ptr = &((*i)->next_mac);
-        }
-        else
-        {
-            hi = m;
-        }
     }
 
-    return lo_ptr;
+    return ret;
 }
 
 void send_beacon_reports(ap *a, int id) {
@@ -667,7 +640,7 @@ int kick_clients(ap* kicking_ap, uint32_t id) {
 
         int do_kick = 0;
 
-        if (mac_in_maclist(j->client_addr)) {
+        if (mac_find_entry(j->client_addr)) {
             dawnlog_info("Station " MACSTR ": Suppressing check due to MAC list entry\n", MAC2STR(j->client_addr.u8));
         }
         else {
@@ -1391,24 +1364,7 @@ void insert_macs_from_file() {
         dawnlog_debug("Retrieved line of length %zu :\n", read);
         dawnlog_debug("%s", line);
 
-        // Need to scanf to an array of ints as there is no byte format specifier
+        insert_to_maclist(str2mac(line));
-        int tmp_int_mac[ETH_ALEN];
-        sscanf(line, MACSTR, STR2MAC(tmp_int_mac));
-
-        struct mac_entry_s* new_mac = dawn_malloc(sizeof(struct mac_entry_s));
-        if (new_mac == NULL)
-        {
-            dawnlog_error("malloc of MAC struct failed!\n");
-        }
-        else
-        {
-            new_mac->next_mac = NULL;
-            for (int i = 0; i < ETH_ALEN; ++i) {
-                new_mac->mac.u8[i] = (uint8_t)tmp_int_mac[i];
-            }
-
-            insert_to_mac_array(new_mac, NULL);
-        }
 
 #ifdef DAWN_MEMORY_AUDITING
         char* old_line = line;
@@ -1444,17 +1400,12 @@ void insert_macs_from_file() {
 
 
 // TODO: This list only ever seems to get longer.  Why do we need it?
-int insert_to_maclist(struct dawn_mac mac) {
+struct mac_entry_s *insert_to_maclist(struct dawn_mac mac) {
-int ret = 0;
-struct mac_entry_s** i = mac_find_first_entry(mac);
-
     dawnlog_debug_func("Entering...");
 
-    if (*i != NULL && mac_is_equal_bb((*i)->mac, mac))
+    struct mac_entry_s* new_mac = NULL;
-    {
+
-        ret = -1;
+    if (mac_find_entry(mac) == NULL)
-    }
-    else
     {
         struct mac_entry_s* new_mac = dawn_malloc(sizeof(struct mac_entry_s));
         if (new_mac == NULL)
@@ -1463,61 +1414,15 @@ struct mac_entry_s** i = mac_find_first_entry(mac);
         }
         else
         {
-            new_mac->next_mac = NULL;
             new_mac->mac = mac;
 
-            insert_to_mac_array(new_mac, i);
+            new_mac->next_mac = mac_set;
+            mac_set = new_mac;
+            mac_set_last++;
         }
     }
 
-    return ret;
+    return new_mac;
-}
-
-// TODO: How big is it in a large network?
-int mac_in_maclist(struct dawn_mac mac) {
-int ret = 0;
-struct mac_entry_s** i = mac_find_first_entry(mac);
-
-    dawnlog_debug_func("Entering...");
-
-    if (*i != NULL && mac_is_equal_bb((*i)->mac, mac))
-    {
-        ret = 1;
-    }
-
-    return ret;
-}
-
-
-struct mac_entry_s* insert_to_mac_array(struct mac_entry_s* entry, struct mac_entry_s** insert_pos) {
-    dawnlog_debug_func("Entering...");;
-
-    if (insert_pos == NULL)
-        insert_pos = mac_find_first_entry(entry->mac);
-
-    entry->next_mac = *insert_pos;
-    *insert_pos = entry;
-    mac_set_last++;
-
-    return entry;
-}
-
-void mac_array_delete(struct mac_entry_s* entry) {
-
-    struct mac_entry_s** i;
-
-    dawnlog_debug_func("Entering...");;
-
-    for (i = &mac_set; *i != NULL; i = &((*i)->next_mac)) {
-        if (*i == entry) {
-            *i = entry->next_mac;
-            mac_set_last--;
-            dawn_free(entry);
-            entry = NULL;
-        }
-    }
-
-    return;
 }
 
 void print_probe_entry(int level, probe_entry *entry) {

src/test/test_storage.c

@@ -665,7 +665,7 @@ static int consume_actions(int argc, char* argv[], int harness_verbosity)
                 struct dawn_mac mac0;
 
                 hwaddr_aton(argv[1], mac0.u8);
-                printf("Looking for MAC %s - result %d\n", argv[1], mac_in_maclist(mac0));
+                printf("Looking for MAC %s - result %d\n", argv[1], mac_find_entry(mac0) == NULL);
             }
         }
         else if (strcmp(*argv, "ap") == 0)

src/utils/mac_utils.c

@@ -6,6 +6,9 @@
 #include "utils.h"
 #include "mac_utils.h"
 
+// Used as a filler where a value is required but not used functionally
+const struct dawn_mac dawn_mac_null = { .u8 = {0,0,0,0,0,0} };
+
 // source: https://elixir.bootlin.com/linux/v4.9/source/lib/hexdump.c#L28
 // based on: hostapd src/utils/common.c
 int hwaddr_aton(const char* txt, uint8_t* addr) {
@@ -46,6 +49,22 @@ int hwaddr_aton(const char* txt, uint8_t* addr) {
     return 0;
 }
 
+struct dawn_mac str2mac(char* s)
+{
+    // Return something testable if sscanf() fails
+    struct dawn_mac tmp_mac = { .u8 = {0,0,0,0,0,0} };
+
+    // Need to scanf to an array of ints as there is no byte format specifier
+    int tmp_int_mac[ETH_ALEN];
+    if (sscanf(s, MACSTR, STR2MAC(tmp_int_mac)) == 6)
+    {
+        for (int i = 0; i < ETH_ALEN; ++i)
+            tmp_mac.u8[i] = (uint8_t)tmp_int_mac[i];
+    }
+
+    return tmp_mac;
+}
+
 void write_mac_to_file(char* path, struct dawn_mac addr) {
     FILE* f = fopen(path, "a");
     if (f == NULL)

src/utils/ubus.c

@@ -368,7 +368,7 @@ bool discard_entry = true;
     if (dawn_metric.eval_auth_req <= 0) {
         dawnlog_trace("Allow authentication due to not evaluating requests");
     }
-    else if (mac_in_maclist(auth_req->client_addr)) {
+    else if (mac_find_entry(auth_req->client_addr)) {
         dawnlog_trace("Allow authentication due to mac_in_maclist()");
     }
     else {
@@ -389,12 +389,6 @@ bool discard_entry = true;
             dawnlog_trace("Deny authentication due to no probe entry");
             deny_request = 1;
         }
-#if 0
-        // Already know this is false from outer test above
-        else if (mac_in_maclist(probe_req_updated->client_addr)) {
-            dawnlog_trace("Short cut due to mac_in_maclist()");
-        }
-#endif
         else if (tmp->counter < dawn_metric.min_probe_count) {
             dawnlog_trace("Deny authentication due to low probe count");
             deny_request = 1;
@@ -447,7 +441,7 @@ int discard_entry = true;
     if (dawn_metric.eval_assoc_req <= 0) {
         dawnlog_trace("Allow association due to not evaluating requests");
     }
-    else if (mac_in_maclist(assoc_req->client_addr)) {
+    else if (mac_find_entry(assoc_req->client_addr)) {
         dawnlog_trace("Allow association due to mac_in_maclist()");
     } else {
         pthread_mutex_lock(&probe_array_mutex);
@@ -467,12 +461,6 @@ int discard_entry = true;
             dawnlog_trace("Deny association due to no probe entry found");
             deny_request = 1;
         }
-#if 0
-        // Already know this is false from outer test above
-        else if (mac_in_maclist(tmp->client_addr)) {
-            dawnlog_trace("Allow due to mac_in_maclist()");
-        }
-#endif
         else if (tmp->counter < dawn_metric.min_probe_count) {
             dawnlog_trace("Deny association due to low probe count");
             deny_request = 1;
@@ -539,7 +527,7 @@ static int handle_probe_req(struct blob_attr* msg) {
         if (dawn_metric.eval_probe_req <= 0) {
             dawnlog_trace("Allow probe due to not evaluating requests");
         }
-        else if (mac_in_maclist(probe_req_updated->client_addr)) {
+        else if (mac_find_entry(probe_req_updated->client_addr)) {
             dawnlog_trace("Allow probe due to mac_in_maclist()");
         }
         else if (probe_req_updated->counter < dawn_metric.min_probe_count) {
@@ -1295,7 +1283,8 @@ int parse_add_mac_to_file(struct blob_attr *msg) {
         struct dawn_mac addr;
         hwaddr_aton(blobmsg_data(attr), addr.u8);
 
-        if (insert_to_maclist(addr) == 0) {
+        // Returns item if it was new and added
+        if (insert_to_maclist(addr)) {
             // TODO: File can grow arbitarily large.  Resource consumption risk.
             // TODO: Consolidate use of file across source: shared resource for name, single point of access?
             write_mac_to_file("/tmp/dawn_mac_list", addr);