esp_netif: added locking for netif list management, unit tests to use unique if_keys, updated comments

components/esp_netif/README.md

@@ -57,7 +57,7 @@ Overall application interaction with communication media and network stack
 ###  C) ESP-NETIF, former tcpip_adapter
 * init API (new, configure)
 * IO API: for passing data between IO driver and network stack
-* event/actiona API (esp-netif lifecycle management)
+* event/action API (esp-netif lifecycle management)
   - building blocks for designing event handlers
 * setters, getters
 * network stack abstraction: enabling user interaction with TCP/IP stack

components/esp_netif/esp_netif_handlers.c

@@ -1,4 +1,4 @@
-// Copyright 2018 Espressif Systems (Shanghai) PTE LTD
+// Copyright 2019 Espressif Systems (Shanghai) PTE LTD
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.

components/esp_netif/esp_netif_objects.c

@@ -15,6 +15,10 @@
 #include "esp_netif.h"
 #include "sys/queue.h"
 #include "esp_log.h"
+#include "freertos/FreeRTOS.h"
+#include "freertos/semphr.h"
+#include "esp_netif_private.h"
+#include <string.h>
 
 //
 // Purpose of this module is to provide list of esp-netif structures
@@ -32,15 +36,38 @@ struct slist_netifs_s {
 SLIST_HEAD(slisthead, slist_netifs_s) s_head = { .slh_first = NULL, };
 
 static size_t s_esp_netif_counter = 0;
+static xSemaphoreHandle  s_list_lock = NULL;
 
 ESP_EVENT_DEFINE_BASE(IP_EVENT);
 
+esp_err_t esp_netif_list_lock(void)
+{
+    if (s_list_lock == NULL) {
+        s_list_lock = xSemaphoreCreateMutex();
+        if (s_list_lock == NULL) {
+            return ESP_ERR_NO_MEM;
+        }
+    }
+    xSemaphoreTake(s_list_lock, portMAX_DELAY);
+    return ESP_OK;
+}
+
+void esp_netif_list_unlock(void)
+{
+    assert(s_list_lock);
+    xSemaphoreGive(s_list_lock);
+    if (s_esp_netif_counter == 0) {
+        vQueueDelete(s_list_lock);
+        s_list_lock = NULL;
+    }
+}
 
 //
 // List manipulation functions
 //
 esp_err_t esp_netif_add_to_list(esp_netif_t *netif)
 {
+    esp_err_t ret;
     struct slist_netifs_s *item = calloc(1, sizeof(struct slist_netifs_s));
     ESP_LOGD(TAG, "%s %p", __func__, netif);
     if (item == NULL) {
@@ -48,9 +75,14 @@ esp_err_t esp_netif_add_to_list(esp_netif_t *netif)
     }
     item->netif = netif;
 
+    if ((ret = esp_netif_list_lock()) != ESP_OK) {
+        return ret;
+    }
+
     SLIST_INSERT_HEAD(&s_head, item, next);
     ++s_esp_netif_counter;
     ESP_LOGD(TAG, "%s netif added successfully (total netifs: %d)", __func__, s_esp_netif_counter);
+    esp_netif_list_unlock();
     return ESP_OK;
 }
 
@@ -58,7 +90,12 @@ esp_err_t esp_netif_add_to_list(esp_netif_t *netif)
 esp_err_t esp_netif_remove_from_list(esp_netif_t *netif)
 {
     struct slist_netifs_s *item;
+    esp_err_t ret;
+    if ((ret = esp_netif_list_lock()) != ESP_OK) {
+        return ret;
+    }
     ESP_LOGV(TAG, "%s %p", __func__, netif);
+
     SLIST_FOREACH(item, &s_head, next) {
         if (item->netif == netif) {
             SLIST_REMOVE(&s_head, item, slist_netifs_s, next);
@@ -66,9 +103,11 @@ esp_err_t esp_netif_remove_from_list(esp_netif_t *netif)
             --s_esp_netif_counter;
             ESP_LOGD(TAG, "%s netif successfully removed (total netifs: %d)", __func__, s_esp_netif_counter);
             free(item);
+            esp_netif_list_unlock();
             return ESP_OK;
         }
     }
+    esp_netif_list_unlock();
     return ESP_ERR_NOT_FOUND;
 }
 
@@ -78,6 +117,19 @@ size_t esp_netif_get_nr_of_ifs(void)
 }
 
 esp_netif_t* esp_netif_next(esp_netif_t* netif)
+{
+    esp_err_t ret;
+    esp_netif_t* result;
+    if ((ret = esp_netif_list_lock()) != ESP_OK) {
+        ESP_LOGE(TAG, "Failed to lock esp-netif list with %d", ret);
+        return NULL;
+    }
+    result = esp_netif_next_unsafe(netif);
+    esp_netif_list_unlock();
+    return result;
+}
+
+esp_netif_t* esp_netif_next_unsafe(esp_netif_t* netif)
 {
     ESP_LOGV(TAG, "%s %p", __func__, netif);
     struct slist_netifs_s *item;
@@ -93,4 +145,43 @@ esp_netif_t* esp_netif_next(esp_netif_t* netif)
         }
     }
     return NULL;
+}
+
+bool esp_netif_is_netif_listed(esp_netif_t *esp_netif)
+{
+    esp_err_t ret;
+    if ((ret = esp_netif_list_lock()) != ESP_OK) {
+        ESP_LOGE(TAG, "Failed to lock esp-netif list with %d", ret);
+        return NULL;
+    }
+
+    // looking for the netif in the list of registered interfaces
+    esp_netif_t *it = esp_netif_next_unsafe(NULL);
+    do {
+        if (it && it == esp_netif) {
+            esp_netif_list_unlock();
+            return true;
+        }
+    } while (NULL != (it = esp_netif_next_unsafe(it)));
+    esp_netif_list_unlock();
+    return false;
+}
+
+esp_netif_t *esp_netif_get_handle_from_ifkey(const char *if_key)
+{
+    esp_err_t ret;
+    if ((ret = esp_netif_list_lock()) != ESP_OK) {
+        ESP_LOGE(TAG, "Failed to lock esp-netif list with %d", ret);
+        return NULL;
+    }
+
+    esp_netif_t *esp_netif = esp_netif_next_unsafe(NULL);
+    do {
+        if (esp_netif && strcmp(if_key, esp_netif_get_ifkey(esp_netif))==0) {
+            esp_netif_list_unlock();
+            return esp_netif;
+        }
+    } while (NULL != (esp_netif = esp_netif_next_unsafe(esp_netif)));
+    esp_netif_list_unlock();
+    return NULL;
 }

components/esp_netif/include/esp_netif.h

@@ -1,4 +1,4 @@
-// Copyright 2015-2016 Espressif Systems (Shanghai) PTE LTD
+// Copyright 2019 Espressif Systems (Shanghai) PTE LTD
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.

components/esp_netif/loopback/esp_netif_loopback.c

@@ -427,17 +427,6 @@ const char *esp_netif_get_desc(esp_netif_t *esp_netif)
     return esp_netif->if_desc;
 }
 
-esp_netif_t *esp_netif_get_handle_from_ifkey(const char *if_key)
-{
-    esp_netif_t *esp_netif = esp_netif_next(NULL);
-    do {
-        if (esp_netif && strcmp(if_key, esp_netif->if_key)==0) {
-            return esp_netif;
-        }
-    } while (NULL != (esp_netif = esp_netif_next(esp_netif)));
-    return NULL;
-}
-
 uint32_t esp_netif_get_event_id(esp_netif_t *esp_netif, esp_netif_ip_event_type_t event_type)
 {
     return 0;

components/esp_netif/lwip/esp_netif_lwip.c

@@ -155,17 +155,12 @@ static inline esp_err_t esp_netif_lwip_ipc_call(esp_netif_api_fn fn, esp_netif_t
  */
 static esp_netif_t* esp_netif_is_active(esp_netif_t *arg)
 {
-    esp_netif_t *esp_netif = NULL;
     // looking for the netif in the list of registered interfaces
     //  as it might have already been destroyed
-    esp_netif_t *nif = esp_netif_next(NULL);
-    do {
-        if (nif && nif == arg) {
-            esp_netif = nif;
-            break;
-        }
-    } while (NULL != (nif = esp_netif_next(nif)));
-    return esp_netif;
+    if (esp_netif_is_netif_listed(arg)) {
+        return arg;
+    }
+    return NULL;
 }
 
 /**
@@ -195,8 +190,9 @@ static void esp_netif_update_default_netif(esp_netif_t *esp_netif, esp_netif_act
         default:
         case ESP_NETIF_STOPPED:
         {
-            esp_netif_t *netif = esp_netif_next(NULL);
             s_last_default_esp_netif = NULL;
+            esp_netif_list_lock();
+            esp_netif_t *netif = esp_netif_next_unsafe(NULL);
             while (netif) {
                 if (esp_netif_is_netif_up(netif)) {
                     if (s_last_default_esp_netif && esp_netif_is_netif_up(s_last_default_esp_netif)) {
@@ -208,8 +204,9 @@ static void esp_netif_update_default_netif(esp_netif_t *esp_netif, esp_netif_act
                         s_last_default_esp_netif = netif;
                     }
                 }
-                netif = esp_netif_next(netif);
+                netif = esp_netif_next_unsafe(netif);
             }
+            esp_netif_list_unlock();
             if (s_last_default_esp_netif && esp_netif_is_netif_up(s_last_default_esp_netif)) {
                 netif_set_default(s_last_default_esp_netif->lwip_netif);
             }
@@ -1362,17 +1359,6 @@ const char *esp_netif_get_desc(esp_netif_t *esp_netif)
     return esp_netif->if_desc;
 }
 
-esp_netif_t *esp_netif_get_handle_from_ifkey(const char *if_key)
-{
-    esp_netif_t *esp_netif = esp_netif_next(NULL);
-    do {
-        if (esp_netif && strcmp(if_key, esp_netif->if_key)==0) {
-            return esp_netif;
-        }
-    } while (NULL != (esp_netif = esp_netif_next(esp_netif)));
-    return NULL;
-}
-
 uint32_t esp_netif_get_event_id(esp_netif_t *esp_netif, esp_netif_ip_event_type_t event_type)
 {
     switch(event_type) {

components/esp_netif/private_include/esp_netif_private.h

@@ -111,4 +111,38 @@ esp_err_t esp_netif_add_to_list(esp_netif_t* netif);
  */
 esp_err_t esp_netif_remove_from_list(esp_netif_t* netif);
 
+/**
+ * @brief Iterates over list of interfaces without list locking. Returns first netif if NULL given as parameter
+ *
+ * Used for bulk search loops to avoid locking and unlocking every iteration. esp_netif_list_lock and esp_netif_list_unlock
+ * must be used to guard the search loop
+ *
+ * @param[in]  esp_netif Handle to esp-netif instance
+ *
+ * @return First netif from the list if supplied parameter is NULL, next one otherwise
+ */
+esp_netif_t* esp_netif_next_unsafe(esp_netif_t* netif);
+
+/**
+ * @brief Locking network interface list. Use only in connection with esp_netif_next_unsafe
+ *
+ * @return ESP_OK on success, specific mutex error if failed to lock
+ */
+esp_err_t esp_netif_list_lock(void);
+
+/**
+ * @brief Unlocking network interface list. Use only in connection with esp_netif_next_unsafe
+ *
+ */
+void esp_netif_list_unlock(void);
+
+/**
+ * @brief Iterates over list of registered interfaces to check if supplied netif is listed
+ *
+ * @param esp_netif network interface to check
+ *
+ * @return true if supplied interface is listed
+ */
+bool esp_netif_is_netif_listed(esp_netif_t *esp_netif);
+
 #endif //_ESP_NETIF_PRIVATE_H_

components/esp_netif/test/test_esp_netif.c

@@ -2,7 +2,7 @@
 #include "esp_netif.h"
 #include "esp_wifi.h"
 
-TEST_CASE("esp_netif: init and destroy", "[esp_netif][leaks=0]")
+TEST_CASE("esp_netif: init and destroy", "[esp_netif]")
 {
     esp_netif_config_t cfg = ESP_NETIF_DEFAULT_WIFI_STA();
     esp_netif_t *esp_netif = esp_netif_new(NULL);
@@ -36,12 +36,15 @@ TEST_CASE("esp_netif: get from if_key", "[esp_netif][leaks=0]")
 
 TEST_CASE("esp_netif: create and delete multiple netifs", "[esp_netif][leaks=0]")
 {
-    const int nr_of_netifs = 10;
+    // interface key has to be a unique identifier
+    const char* if_keys[] = { "if1", "if2", "if3", "if4", "if5", "if6", "if7", "if8", "if9" };
+    const int nr_of_netifs = sizeof(if_keys)/sizeof(char*);
     esp_netif_t *netifs[nr_of_netifs];
-    esp_netif_config_t cfg = ESP_NETIF_DEFAULT_WIFI_STA();
 
     // create 10 wifi stations
     for (int i=0; i<nr_of_netifs; ++i) {
+        esp_netif_inherent_config_t base_netif_config = { .if_key = if_keys[i]};
+        esp_netif_config_t cfg = { .base = &base_netif_config, .stack = ESP_NETIF_NETSTACK_DEFAULT_WIFI_STA };
         netifs[i] = esp_netif_new(&cfg);
         TEST_ASSERT_NOT_NULL(netifs[i]);
     }

components/esp_wifi/include/esp_wifi_netif.h

@@ -66,7 +66,7 @@ esp_err_t esp_wifi_get_if_mac(wifi_netif_driver_t ifx, uint8_t mac[6]);
  *      - true if ready after intertace started (typically Access Point type)
  *      - false if ready once intertace connected (typically for Station type)
  */
-bool esp_wifi_is_if_ready_when_stared(wifi_netif_driver_t ifx);
+bool esp_wifi_is_if_ready_when_started(wifi_netif_driver_t ifx);
 
 /**
  * @brief Register interface receive callback function with argument

components/esp_wifi/src/wifi_default.c

@@ -1,4 +1,4 @@
-// Copyright 2018 Espressif Systems (Shanghai) PTE LTD
+// Copyright 2019 Espressif Systems (Shanghai) PTE LTD
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -50,7 +50,7 @@ static void wifi_start(void *esp_netif, esp_event_base_t base, int32_t event_id,
     }
     ESP_LOGD(TAG, "WIFI mac address: %x %x %x %x %x %x", mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
 
-    if (esp_wifi_is_if_ready_when_stared(driver)) {
+    if (esp_wifi_is_if_ready_when_started(driver)) {
         if ((ret = esp_wifi_register_if_rxcb(driver,  esp_netif_receive, esp_netif)) != ESP_OK) {
             ESP_LOGE(TAG, "esp_wifi_register_if_rxcb for if=%p failed with %d", driver, ret);
             return;
@@ -86,7 +86,7 @@ static void wifi_default_action_sta_connected(void *arg, esp_event_base_t base,
         esp_netif_t *esp_netif = s_wifi_netifs[WIFI_IF_STA];
         wifi_netif_driver_t driver = esp_netif_get_io_driver(esp_netif);
 
-        if (!esp_wifi_is_if_ready_when_stared(driver)) {
+        if (!esp_wifi_is_if_ready_when_started(driver)) {
             // if interface not ready when started, rxcb to be registered on connection
             if ((ret = esp_wifi_register_if_rxcb(driver,  esp_netif_receive, esp_netif)) != ESP_OK) {
                 ESP_LOGE(TAG, "esp_wifi_register_if_rxcb for if=%p failed with %d", driver, ret);

components/esp_wifi/src/wifi_netif.c

@@ -1,4 +1,4 @@
-// Copyright 2018 Espressif Systems (Shanghai) PTE LTD
+// Copyright 2019 Espressif Systems (Shanghai) PTE LTD
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -100,7 +100,7 @@ esp_err_t esp_wifi_get_if_mac(wifi_netif_driver_t ifx, uint8_t mac[6])
     return esp_wifi_get_mac(wifi_interface, mac);
 }
 
-bool esp_wifi_is_if_ready_when_stared(wifi_netif_driver_t ifx)
+bool esp_wifi_is_if_ready_when_started(wifi_netif_driver_t ifx)
 {
     // WiFi rxcb to be register wifi rxcb on start for AP only, station gets it registered on connect event
     return (ifx->wifi_if == WIFI_IF_AP);