From 88d18e9a402b351ac79789606f406a45e8fc25ec Mon Sep 17 00:00:00 2001 From: David Cermak Date: Tue, 3 Oct 2023 17:38:18 +0200 Subject: [PATCH] fix(esp_netif): Mark esp_netif_next deprecated and fix usages * Uses netif_find_if() in IPv6 examples * Fixes esp_netif_next() usage in L2TAP --- components/esp_netif/include/esp_netif.h | 22 ++++- components/esp_netif/lwip/esp_netif_lwip.c | 2 +- .../private_include/esp_netif_private.h | 25 ------ .../esp_netif/vfs_l2tap/esp_vfs_l2tap.c | 15 ++-- .../protocol_examples_common/connect.c | 28 +++--- .../sockets/icmpv6_ping/main/icmpv6_ping.c | 38 +++++---- .../sockets/tcp_client/main/tcp_client_v6.c | 85 ++++++------------- 7 files changed, 97 insertions(+), 118 deletions(-) diff --git a/components/esp_netif/include/esp_netif.h b/components/esp_netif/include/esp_netif.h index 7a698175c4..32a068491b 100644 --- a/components/esp_netif/include/esp_netif.h +++ b/components/esp_netif/include/esp_netif.h @@ -979,11 +979,29 @@ int32_t esp_netif_get_event_id(esp_netif_t *esp_netif, esp_netif_ip_event_type_t * 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() * + * @note This API is deprecated. Please 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); +esp_netif_t *esp_netif_next(esp_netif_t *esp_netif) +__attribute__((deprecated("use esp_netif_next_unsafe() either directly or via esp_netif_tcpip_exec"))); + +/** + * @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 @@ -992,7 +1010,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 0b41dda795..a3d230aa13 100644 --- a/components/esp_netif/lwip/esp_netif_lwip.c +++ b/components/esp_netif/lwip/esp_netif_lwip.c @@ -812,7 +812,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 f4f76815b2..8c80fe1451 100644 --- a/components/esp_netif/private_include/esp_netif_private.h +++ b/components/esp_netif/private_include/esp_netif_private.h @@ -107,31 +107,6 @@ esp_err_t esp_netif_add_to_list_unsafe(esp_netif_t* netif); */ esp_err_t esp_netif_remove_from_list_unsafe(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 * 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 d40870eb02..6abc2a3d24 100644 --- a/examples/common_components/protocol_examples_common/connect.c +++ b/examples/common_components/protocol_examples_common/connect.c @@ -43,23 +43,22 @@ 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; - 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)); #if CONFIG_LWIP_IPV4 @@ -78,6 +77,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 d9db5f16ac..5f152838ab 100644 --- a/examples/protocols/sockets/icmpv6_ping/main/icmpv6_ping.c +++ b/examples/protocols/sockets/icmpv6_ping/main/icmpv6_ping.c @@ -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); diff --git a/examples/protocols/sockets/tcp_client/main/tcp_client_v6.c b/examples/protocols/sockets/tcp_client/main/tcp_client_v6.c index c150177b4a..a8af37b740 100644 --- a/examples/protocols/sockets/tcp_client/main/tcp_client_v6.c +++ b/examples/protocols/sockets/tcp_client/main/tcp_client_v6.c @@ -84,78 +84,48 @@ static int get_src_iface(char *interface) } #else -static esp_netif_t *get_esp_netif_from_iface(char *interface_i) -{ - esp_netif_t *netif = NULL; - esp_err_t ret = ESP_FAIL; - char iface[10]; - - // 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, iface); - if ((ESP_FAIL == ret) || (NULL == netif)) { - ESP_LOGE(TAG, "No interface available"); - return NULL; - } - - if (0 == strcmp(interface_i, iface)) { - return netif; - } - } - - return NULL; -} - - /** * @brief In case of Auto mode returns the interface name with a valid IPv6 address or * In case the user has specified interface, validates and returns the interface name. * - * @param[out] interface Name of the interface in as a string. + * This function is a predicate for esp_netif_find_if() API, and it uses the underlying + * network interface name as a context parameter * - * @return 0 incase of success. + * @return true if we found the appropriate interface, false if not */ -static int get_src_iface(char *interface) +static bool choose_netif(esp_netif_t *netif, void* ctx) { - esp_netif_t *netif = NULL; - esp_err_t ret = ESP_FAIL; - int ip6_addrs_count = 0; + char *interface = ctx; esp_ip6_addr_t ip6[LWIP_IPV6_NUM_ADDRESSES]; - // 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); - - if ((ESP_FAIL == ret) || (NULL == netif)) { - ESP_LOGE(TAG, "No interface available"); - return -1; - } + esp_err_t ret = esp_netif_get_netif_impl_name(netif, interface); + if ((ESP_FAIL == ret) || (NULL == netif)) { + ESP_LOGE(TAG, "No interface available"); + return false; + } #if defined(CONFIG_EXAMPLE_USER_SPECIFIED_IFACE) - if (!strcmp(CONFIG_EXAMPLE_USER_SPECIFIED_IFACE_NAME, interface)) { + if (!strcmp(CONFIG_EXAMPLE_USER_SPECIFIED_IFACE_NAME, interface)) { ESP_LOGI(TAG, "Interface: %s", interface); - return 0; + return true; } #else - ip6_addrs_count = esp_netif_get_all_ip6(netif, ip6); - for (int j = 0; j < ip6_addrs_count; ++j) { - esp_ip6_addr_type_t ipv6_type = esp_netif_ip6_get_addr_type(&(ip6[j])); + int ip6_addrs_count = esp_netif_get_all_ip6(netif, ip6); + for (int j = 0; j < ip6_addrs_count; ++j) { + esp_ip6_addr_type_t ipv6_type = esp_netif_ip6_get_addr_type(&(ip6[j])); - if ((ESP_IP6_ADDR_IS_GLOBAL == ipv6_type) || - (ESP_IP6_ADDR_IS_UNIQUE_LOCAL == ipv6_type) || - (ESP_IP6_ADDR_IS_LINK_LOCAL == ipv6_type)) { - // Break as we have the source address - ESP_LOGI(TAG, "Interface: %s", interface); - return 0; - } + if ((ESP_IP6_ADDR_IS_GLOBAL == ipv6_type) || + (ESP_IP6_ADDR_IS_UNIQUE_LOCAL == ipv6_type) || + (ESP_IP6_ADDR_IS_LINK_LOCAL == ipv6_type)) { + // Break as we have the source address + ESP_LOGI(TAG, "Interface: %s", interface); + return true; } -#endif // #if defined(CONFIG_EXAMPLE_USER_SPECIFIED_IFACE) } - - return -1; +#endif // #if defined(CONFIG_EXAMPLE_USER_SPECIFIED_IFACE) + return false; } + #endif // #if defined(CONFIG_IDF_TARGET_LINUX) @@ -191,12 +161,11 @@ void tcp_client(void) } ESP_LOGI(TAG, "Socket created, connecting to %s:%d", host_ip, PORT); +#if defined(CONFIG_IDF_TARGET_LINUX) if (0 != get_src_iface(interface)) { ESP_LOGE(TAG, "Interface: Unavailable"); break; } - -#if defined(CONFIG_IDF_TARGET_LINUX) memset (&ifr, 0, sizeof(ifr)); snprintf (ifr.ifr_name, sizeof (ifr.ifr_name), "%s", interface); if (ioctl (sock, SIOCGIFINDEX, &ifr) < 0) { @@ -208,13 +177,13 @@ void tcp_client(void) ESP_LOGI(TAG, "Interface index: %d", dest_addr.sin6_scope_id); #endif #else - if (NULL == (netif = get_esp_netif_from_iface(interface))) { + if (NULL == (netif = esp_netif_find_if(choose_netif, interface))) { ESP_LOGE(TAG, "Failed to find interface "); break; } #if defined(CONFIG_EXAMPLE_IPV6) dest_addr.sin6_scope_id = esp_netif_get_netif_impl_index(netif); - ESP_LOGI(TAG, "Interface index: %d", dest_addr.sin6_scope_id); + ESP_LOGI(TAG, "Interface index: %" PRIu32, dest_addr.sin6_scope_id); #endif #endif