From 41cd40e7f3d7e81d3f10ea04c843603220126d62 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Tue, 3 Oct 2023 17:38:18 +0200 Subject: [PATCH] fix(esp_netif): Make esp_netif_next_unsafe() public and update usage Updates usage of esp_netif_next() in examples and tests * Uses netif_find_if() in IPv6 examples * Fixes esp_netif_next() usage in L2TAP --- components/esp_netif/include/esp_netif.h | 23 ++++++++++- components/esp_netif/lwip/esp_netif_lwip.c | 2 +- .../private_include/esp_netif_private.h | 15 +------ .../esp_netif/vfs_l2tap/esp_vfs_l2tap.c | 15 ++++--- .../protocol_examples_common/connect.c | 32 +++++++++------ .../sockets/icmpv6_ping/main/icmpv6_ping.c | 40 +++++++++++-------- 6 files changed, 77 insertions(+), 50 deletions(-) diff --git a/components/esp_netif/include/esp_netif.h b/components/esp_netif/include/esp_netif.h index 2b1664ce7e..8037cf0bcb 100644 --- a/components/esp_netif/include/esp_netif.h +++ b/components/esp_netif/include/esp_netif.h @@ -939,12 +939,33 @@ int32_t esp_netif_get_event_id(esp_netif_t *esp_netif, esp_netif_ip_event_type_t /** * @brief Iterates over list of interfaces. Returns first netif if NULL given as parameter * + * @note This API doesn't lock the list, nor the TCPIP context, as this it's usually required + * to get atomic access between iteration steps rather that within a single iteration. + * Therefore it is recommended to iterate over the interfaces inside esp_netif_tcpip_exec() + * + * You can use esp_netif_next_unsafe() directly if all the system + * interfaces are under your control and you can safely iterate over them. + * Otherwise, iterate over interfaces using esp_netif_tcpip_exec(), or use esp_netif_find_if() + * to search in the list of netifs with defined predicate. + * * @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(esp_netif_t *esp_netif); +/** + * @brief Iterates over list of interfaces without list locking. Returns first netif if NULL given as parameter + * + * Used for bulk search loops within TCPIP context, e.g. using esp_netif_tcpip_exec(), or if we're sure + * that the iteration is safe from our application perspective (e.g. no interface is removed between iterations) + * + * @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* esp_netif); + /** * @brief Predicate callback for esp_netif_find_if() used to find interface * which meets defined criteria @@ -952,7 +973,7 @@ esp_netif_t *esp_netif_next(esp_netif_t *esp_netif); typedef bool (*esp_netif_find_predicate_t)(esp_netif_t *netif, void *ctx); /** - * @brief Return a netif pointer for the interface that meets criteria defined + * @brief Return a netif pointer for the first interface that meets criteria defined * by the callback * * @param fn Predicate function returning true for the desired interface diff --git a/components/esp_netif/lwip/esp_netif_lwip.c b/components/esp_netif/lwip/esp_netif_lwip.c index 37c4f5a648..e956f1900b 100644 --- a/components/esp_netif/lwip/esp_netif_lwip.c +++ b/components/esp_netif/lwip/esp_netif_lwip.c @@ -787,7 +787,7 @@ static esp_err_t esp_netif_find_if_api(esp_netif_api_msg_t *msg) { find_if_api_t *find_if_api = msg->data; esp_netif_t *esp_netif = NULL; - while ((esp_netif = esp_netif_next(esp_netif)) != NULL) { + while ((esp_netif = esp_netif_next_unsafe(esp_netif)) != NULL) { if (find_if_api->fn(esp_netif, find_if_api->ctx)) { *msg->p_esp_netif = esp_netif; return ESP_OK; diff --git a/components/esp_netif/private_include/esp_netif_private.h b/components/esp_netif/private_include/esp_netif_private.h index 954dc00b6c..ba0909d917 100644 --- a/components/esp_netif/private_include/esp_netif_private.h +++ b/components/esp_netif/private_include/esp_netif_private.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -103,23 +103,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); -/** - * @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); /** diff --git a/components/esp_netif/vfs_l2tap/esp_vfs_l2tap.c b/components/esp_netif/vfs_l2tap/esp_vfs_l2tap.c index 3a814b791d..69b1959f41 100644 --- a/components/esp_netif/vfs_l2tap/esp_vfs_l2tap.c +++ b/components/esp_netif/vfs_l2tap/esp_vfs_l2tap.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2021-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2021-2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -333,6 +333,12 @@ static int l2tap_close(int fd) return 0; } +// used to find a netif with the attached driver matching the argument +static bool netif_driver_matches(esp_netif_t *netif, void* driver) +{ + return esp_netif_get_io_driver(netif) == driver; +} + static int l2tap_ioctl(int fd, int cmd, va_list args) { esp_netif_t *esp_netif; @@ -383,11 +389,8 @@ static int l2tap_ioctl(int fd, int cmd, va_list args) case L2TAP_G_INTF_DEVICE: ; const char **str_p = va_arg(args, const char **); *str_p = NULL; - esp_netif = NULL; - while ((esp_netif = esp_netif_next(esp_netif)) != NULL) { - if (s_l2tap_sockets[fd].driver_handle == esp_netif_get_io_driver(esp_netif)) { - *str_p = esp_netif_get_ifkey(esp_netif); - } + if ((esp_netif = esp_netif_find_if(netif_driver_matches, s_l2tap_sockets[fd].driver_handle)) != NULL) { + *str_p = esp_netif_get_ifkey(esp_netif); } break; case L2TAP_S_DEVICE_DRV_HNDL: ; diff --git a/examples/common_components/protocol_examples_common/connect.c b/examples/common_components/protocol_examples_common/connect.c index 8a7e28266b..c68d6695e8 100644 --- a/examples/common_components/protocol_examples_common/connect.c +++ b/examples/common_components/protocol_examples_common/connect.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Unlicense OR CC0-1.0 */ @@ -43,26 +43,25 @@ bool example_is_our_netif(const char *prefix, esp_netif_t *netif) return strncmp(prefix, esp_netif_get_desc(netif), strlen(prefix) - 1) == 0; } -esp_netif_t *get_example_netif_from_desc(const char *desc) +static bool netif_desc_matches_with(esp_netif_t *netif, void *ctx) { - esp_netif_t *netif = NULL; - while ((netif = esp_netif_next(netif)) != NULL) { - if (strcmp(esp_netif_get_desc(netif), desc) == 0) { - return netif; - } - } - return netif; + return strcmp(ctx, esp_netif_get_desc(netif)) == 0; } -void example_print_all_netif_ips(const char *prefix) +esp_netif_t *get_example_netif_from_desc(const char *desc) { + return esp_netif_find_if(netif_desc_matches_with, (void*)desc); +} + +static esp_err_t print_all_ips_tcpip(void* ctx) +{ + const char *prefix = ctx; // iterate over active interfaces, and print out IPs of "our" netifs esp_netif_t *netif = NULL; - esp_netif_ip_info_t ip; - for (int i = 0; i < esp_netif_get_nr_of_ifs(); ++i) { - netif = esp_netif_next(netif); + while ((netif = esp_netif_next_unsafe(netif)) != NULL) { if (example_is_our_netif(prefix, netif)) { ESP_LOGI(TAG, "Connected to %s", esp_netif_get_desc(netif)); + esp_netif_ip_info_t ip; ESP_ERROR_CHECK(esp_netif_get_ip_info(netif, &ip)); ESP_LOGI(TAG, "- IPv4 address: " IPSTR ",", IP2STR(&ip.ip)); @@ -76,6 +75,13 @@ void example_print_all_netif_ips(const char *prefix) #endif } } + return ESP_OK; +} + +void example_print_all_netif_ips(const char *prefix) +{ + // Print all IPs in TCPIP context to avoid potential races of removing/adding netifs when iterating over the list + esp_netif_tcpip_exec(print_all_ips_tcpip, (void*) prefix); } diff --git a/examples/protocols/sockets/icmpv6_ping/main/icmpv6_ping.c b/examples/protocols/sockets/icmpv6_ping/main/icmpv6_ping.c index 695b67144f..a7005b4d07 100644 --- a/examples/protocols/sockets/icmpv6_ping/main/icmpv6_ping.c +++ b/examples/protocols/sockets/icmpv6_ping/main/icmpv6_ping.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Unlicense OR CC0-1.0 */ @@ -215,34 +215,41 @@ static void send_ping(char *src_addr_str, char *dst_addr_str, char *interface) free(msghdr.msg_control); } +/** + * @brief API struct to pass interface name and source addresses in TCPIP context + * + * @param[out] interface Name of the interface. + * @param[out] src_addr_str Global/Unique local IPv6 address of the interface + */ +typedef struct src_iface_api { + char *interface; + char *src_addr_str; +} src_iface_api_t; /** * @brief Goes over each interface and searches for one Global/Unique local IPv6 address. * Returns the interface name and IPv6 address of the interface in case of success. * - * @param[out] interface Name of the interface. - * @param[out] src_addr_str Global/Unique local IPv6 address of the interface * @return - * >0 : Successfully found an interface with Global/Unique local IPv6 address. - * -1 : Unable to to find a valid interface with Global/Unique local IPv6 address. + * ESP_OK : Successfully found an interface with Global/Unique local IPv6 address. + * ESP_FAIL : Unable to to find a valid interface with Global/Unique local IPv6 address. */ -bool get_src_iface(char *interface, char *src_addr_str) +static esp_err_t get_src_iface(void* ctx) { + src_iface_api_t *api = ctx; esp_netif_t *netif = NULL; int ip6_addrs_count = 0; esp_ip6_addr_t ip6[LWIP_IPV6_NUM_ADDRESSES]; - esp_err_t ret = ESP_FAIL; // Get interface details and own global ipv6 address - for (int i = 0; i < esp_netif_get_nr_of_ifs(); ++i) { - netif = esp_netif_next(netif); - ret = esp_netif_get_netif_impl_name(netif, interface); + while ((netif = esp_netif_next_unsafe(netif)) != NULL) { + esp_err_t ret = esp_netif_get_netif_impl_name(netif, api->interface); if ((ESP_FAIL == ret) || (NULL == netif)) { ESP_LOGE(TAG, "No interface available"); - return false; + return ESP_FAIL; } - ESP_LOGI(TAG, "Interface: %s", interface); + ESP_LOGI(TAG, "Interface: %s", api->interface); ip6_addrs_count = esp_netif_get_all_ip6(netif, ip6); for (int j = 0; j < ip6_addrs_count; ++j) { @@ -252,13 +259,13 @@ bool get_src_iface(char *interface, char *src_addr_str) if ((ESP_IP6_ADDR_IS_GLOBAL == ipv6_type) || (ESP_IP6_ADDR_IS_UNIQUE_LOCAL == ipv6_type)) { // Break as we have the source address - sprintf(src_addr_str, IPV6STR, IPV62STR(ip6[j])); - return true; + sprintf(api->src_addr_str, IPV6STR, IPV62STR(ip6[j])); + return ESP_OK; } } } - return false; + return ESP_FAIL; } @@ -268,7 +275,8 @@ static void ping6_test_task(void *pvParameters) char interface[10]; char dst_addr_str[] = CONFIG_EXAMPLE_DST_IPV6_ADDR; - if (true == get_src_iface(interface, src_addr_str)) { + src_iface_api_t api = { .interface = interface, .src_addr_str = src_addr_str }; + if (esp_netif_tcpip_exec(get_src_iface, &api) == ESP_OK) { ESP_LOGI(TAG, "Source address: %s", src_addr_str); ESP_LOGI(TAG, "Destination address: %s", dst_addr_str); ESP_LOGI(TAG, "Interface name: %s", interface);