From 734b6ab57a5f2ff07e19fea358ddd45607ea200b Mon Sep 17 00:00:00 2001 From: goldsimon Date: Fri, 12 Jan 2018 23:11:38 +0100 Subject: [PATCH] netif: ensure netif_set_addr() only results in one "ext_status_callback" This can be used e.g. in mdns to create one, not multiple "changed" triggers if IP address and netmask change at the same time. --- src/core/netif.c | 169 +++++++++++++++++++++++------------- src/include/lwip/netif.h | 16 +--- test/unit/core/test_netif.c | 111 +++++++++++------------ 3 files changed, 162 insertions(+), 134 deletions(-) diff --git a/src/core/netif.c b/src/core/netif.c index 9bb15c66..139f1b13 100644 --- a/src/core/netif.c +++ b/src/core/netif.c @@ -421,28 +421,29 @@ netif_add(struct netif *netif, } #if LWIP_IPV4 -static void -netif_do_set_ipaddr(struct netif *netif, const ip4_addr_t *ipaddr, u8_t callback) +static int +netif_do_set_ipaddr(struct netif *netif, const ip4_addr_t *ipaddr, ip_addr_t *old_addr) { - ip_addr_t new_addr; - - *ip_2_ip4(&new_addr) = *ipaddr; - IP_SET_TYPE_VAL(new_addr, IPADDR_TYPE_V4); + LWIP_ASSERT("invalid pointer", ipaddr != NULL); + LWIP_ASSERT("invalid pointer", old_addr != NULL); /* address is actually being changed? */ - if (ip4_addr_cmp(ip_2_ip4(&new_addr), netif_ip4_addr(netif)) == 0) { - ip_addr_t old_addr; - ip_addr_copy(old_addr, *netif_ip_addr4(netif)); + if (ip4_addr_cmp(ipaddr, netif_ip4_addr(netif)) == 0) { + ip_addr_t new_addr; + *ip_2_ip4(&new_addr) = *ipaddr; + IP_SET_TYPE_VAL(new_addr, IPADDR_TYPE_V4); + + ip_addr_copy(*old_addr, *netif_ip_addr4(netif)); LWIP_DEBUGF(NETIF_DEBUG | LWIP_DBG_STATE, ("netif_set_ipaddr: netif address being changed\n")); #if LWIP_TCP - tcp_netif_ip_addr_changed(&old_addr, &new_addr); + tcp_netif_ip_addr_changed(old_addr, &new_addr); #endif /* LWIP_TCP */ #if LWIP_UDP - udp_netif_ip_addr_changed(&old_addr, &new_addr); + udp_netif_ip_addr_changed(old_addr, &new_addr); #endif /* LWIP_UDP */ #if LWIP_RAW - raw_netif_ip_addr_changed(&old_addr, &new_addr); + raw_netif_ip_addr_changed(old_addr, &new_addr); #endif /* LWIP_RAW */ mib2_remove_ip4(netif); @@ -456,14 +457,9 @@ netif_do_set_ipaddr(struct netif *netif, const ip4_addr_t *ipaddr, u8_t callback netif_issue_reports(netif, NETIF_REPORT_TYPE_IPV4); NETIF_STATUS_CALLBACK(netif); - if (callback) { -#if LWIP_NETIF_EXT_STATUS_CALLBACK - netif_ext_callback_args_t args; - args.ipv4_changed.old_address = &old_addr; - netif_invoke_ext_callback(netif, LWIP_NSC_IPV4_ADDRESS_CHANGED, &args); -#endif - } + return 1; /* address changed */ } + return 0; /* address unchanged */ } /** @@ -479,6 +475,7 @@ netif_do_set_ipaddr(struct netif *netif, const ip4_addr_t *ipaddr, u8_t callback void netif_set_ipaddr(struct netif *netif, const ip4_addr_t *ipaddr) { + ip_addr_t old_addr; /* Don't propagate NULL pointer (IPv4 ANY) to subsequent functions */ if (ipaddr == NULL) { ipaddr = IP4_ADDR_ANY4; @@ -486,21 +483,26 @@ netif_set_ipaddr(struct netif *netif, const ip4_addr_t *ipaddr) LWIP_ASSERT_CORE_LOCKED(); - netif_do_set_ipaddr(netif, ipaddr, 1); + if (netif_do_set_ipaddr(netif, ipaddr, &old_addr)) { +#if LWIP_NETIF_EXT_STATUS_CALLBACK + netif_ext_callback_args_t args; + args.ipv4_changed.old_address = &old_addr; + netif_invoke_ext_callback(netif, LWIP_NSC_IPV4_ADDRESS_CHANGED, &args); +#endif + } } -static void -netif_do_set_netmask(struct netif *netif, const ip4_addr_t *netmask, u8_t callback) +static int +netif_do_set_netmask(struct netif *netif, const ip4_addr_t *netmask, ip_addr_t *old_nm) { /* address is actually being changed? */ if (ip4_addr_cmp(netmask, netif_ip4_netmask(netif)) == 0) { #if LWIP_NETIF_EXT_STATUS_CALLBACK - netif_ext_callback_args_t args; - ip_addr_t old_addr; - ip_addr_copy(old_addr, *netif_ip_netmask4(netif)); - args.ipv4_nm_changed.old_address = &old_addr; + LWIP_ASSERT("invalid pointer", old_nm != NULL); + ip_addr_copy(*old_nm, *netif_ip_netmask4(netif)); +#else + LWIP_UNUSED_ARG(old_nm); #endif - mib2_remove_route_ip4(0, netif); /* set new netmask to netif */ ip4_addr_set(ip_2_ip4(&netif->netmask), netmask); @@ -512,11 +514,9 @@ netif_do_set_netmask(struct netif *netif, const ip4_addr_t *netmask, u8_t callba ip4_addr2_16(netif_ip4_netmask(netif)), ip4_addr3_16(netif_ip4_netmask(netif)), ip4_addr4_16(netif_ip4_netmask(netif)))); - - if (callback) { - netif_invoke_ext_callback(netif, LWIP_NSC_IPV4_NETMASK_CHANGED, &args); - } + return 1; /* netmask changed */ } + return 0; /* netmask unchanged */ } /** @@ -532,6 +532,12 @@ netif_do_set_netmask(struct netif *netif, const ip4_addr_t *netmask, u8_t callba void netif_set_netmask(struct netif *netif, const ip4_addr_t *netmask) { +#if LWIP_NETIF_EXT_STATUS_CALLBACK + ip_addr_t old_nm_val; + ip_addr_t *old_nm = &old_nm_val; +#else + ip_addr_t *old_nm = NULL; +#endif LWIP_ASSERT_CORE_LOCKED(); /* Don't propagate NULL pointer (IPv4 ANY) to subsequent functions */ @@ -539,19 +545,25 @@ netif_set_netmask(struct netif *netif, const ip4_addr_t *netmask) netmask = IP4_ADDR_ANY4; } - netif_do_set_netmask(netif, netmask, 1); + if (netif_do_set_netmask(netif, netmask, old_nm)) { +#if LWIP_NETIF_EXT_STATUS_CALLBACK + netif_ext_callback_args_t args; + args.ipv4_changed.old_netmask = old_nm; + netif_invoke_ext_callback(netif, LWIP_NSC_IPV4_NETMASK_CHANGED, &args); +#endif + } } -static void -netif_do_set_gw(struct netif *netif, const ip4_addr_t *gw, u8_t callback) +static int +netif_do_set_gw(struct netif *netif, const ip4_addr_t *gw, ip_addr_t *old_gw) { /* address is actually being changed? */ if (ip4_addr_cmp(gw, netif_ip4_gw(netif)) == 0) { #if LWIP_NETIF_EXT_STATUS_CALLBACK - netif_ext_callback_args_t args; - ip_addr_t old_addr; - ip_addr_copy(old_addr, *netif_ip_gw4(netif)); - args.ipv4_gw_changed.old_address = &old_addr; + LWIP_ASSERT("invalid pointer", old_gw != NULL); + ip_addr_copy(*old_gw, *netif_ip_gw4(netif)); +#else + LWIP_UNUSED_ARG(old_gw); #endif ip4_addr_set(ip_2_ip4(&netif->gw), gw); @@ -562,11 +574,9 @@ netif_do_set_gw(struct netif *netif, const ip4_addr_t *gw, u8_t callback) ip4_addr2_16(netif_ip4_gw(netif)), ip4_addr3_16(netif_ip4_gw(netif)), ip4_addr4_16(netif_ip4_gw(netif)))); - - if (callback) { - netif_invoke_ext_callback(netif, LWIP_NSC_IPV4_GATEWAY_CHANGED, &args); - } + return 1; /* gateway changed */ } + return 0; /* gateway unchanged */ } /** @@ -581,6 +591,12 @@ netif_do_set_gw(struct netif *netif, const ip4_addr_t *gw, u8_t callback) void netif_set_gw(struct netif *netif, const ip4_addr_t *gw) { +#if LWIP_NETIF_EXT_STATUS_CALLBACK + ip_addr_t old_gw_val; + ip_addr_t *old_gw = &old_gw_val; +#else + ip_addr_t *old_gw = NULL; +#endif LWIP_ASSERT_CORE_LOCKED(); /* Don't propagate NULL pointer (IPv4 ANY) to subsequent functions */ @@ -588,7 +604,13 @@ netif_set_gw(struct netif *netif, const ip4_addr_t *gw) gw = IP4_ADDR_ANY4; } - netif_do_set_gw(netif, gw, 1); + if (netif_do_set_gw(netif, gw, old_gw)) { +#if LWIP_NETIF_EXT_STATUS_CALLBACK + netif_ext_callback_args_t args; + args.ipv4_changed.old_gw = old_gw; + netif_invoke_ext_callback(netif, LWIP_NSC_IPV4_GATEWAY_CHANGED, &args); +#endif + } } /** @@ -606,8 +628,18 @@ netif_set_addr(struct netif *netif, const ip4_addr_t *ipaddr, const ip4_addr_t * const ip4_addr_t *gw) { #if LWIP_NETIF_EXT_STATUS_CALLBACK - u8_t something_changed = 0; + netif_nsc_reason_t change_reason = LWIP_NSC_NONE; + netif_ext_callback_args_t cb_args; + ip_addr_t old_nm_val; + ip_addr_t old_gw_val; + ip_addr_t *old_nm = &old_nm_val; + ip_addr_t *old_gw = &old_gw_val; +#else + ip_addr_t *old_nm = NULL; + ip_addr_t *old_gw = NULL; #endif + ip_addr_t old_addr; + int remove; LWIP_ASSERT_CORE_LOCKED(); @@ -622,30 +654,43 @@ netif_set_addr(struct netif *netif, const ip4_addr_t *ipaddr, const ip4_addr_t * gw = IP4_ADDR_ANY4; } -#if LWIP_NETIF_EXT_STATUS_CALLBACK - if ((ip4_addr_cmp(ipaddr, netif_ip4_addr(netif)) == 0) || - (ip4_addr_cmp(gw, netif_ip4_gw(netif)) == 0) || - (ip4_addr_cmp(netmask, netif_ip4_netmask(netif)) == 0)) { - something_changed = 1; - } -#endif - - if (ip4_addr_isany(ipaddr)) { + remove = ip4_addr_isany(ipaddr); + if (remove) { /* when removing an address, we have to remove it *before* changing netmask/gw to ensure that tcp RST segment can be sent correctly */ - netif_do_set_ipaddr(netif, ipaddr, 0); - netif_do_set_netmask(netif, netmask, 0); - netif_do_set_gw(netif, gw, 0); - } else { + if (netif_do_set_ipaddr(netif, ipaddr, &old_addr)) { +#if LWIP_NETIF_EXT_STATUS_CALLBACK + change_reason |= LWIP_NSC_IPV4_ADDRESS_CHANGED; + cb_args.ipv4_changed.old_address = &old_addr; +#endif + } + } + if (netif_do_set_netmask(netif, netmask, old_nm)) { +#if LWIP_NETIF_EXT_STATUS_CALLBACK + change_reason |= LWIP_NSC_IPV4_NETMASK_CHANGED; + cb_args.ipv4_changed.old_netmask = old_nm; +#endif + } + if (netif_do_set_gw(netif, gw, old_gw)) { +#if LWIP_NETIF_EXT_STATUS_CALLBACK + change_reason |= LWIP_NSC_IPV4_GATEWAY_CHANGED; + cb_args.ipv4_changed.old_gw = old_gw; +#endif + } + if (!remove) { /* set ipaddr last to ensure netmask/gw have been set when status callback is called */ - netif_do_set_netmask(netif, netmask, 0); - netif_do_set_gw(netif, gw, 0); - netif_do_set_ipaddr(netif, ipaddr, 0); + if (netif_do_set_ipaddr(netif, ipaddr, &old_addr)) { +#if LWIP_NETIF_EXT_STATUS_CALLBACK + change_reason |= LWIP_NSC_IPV4_ADDRESS_CHANGED; + cb_args.ipv4_changed.old_address = &old_addr; +#endif + } } #if LWIP_NETIF_EXT_STATUS_CALLBACK - if (something_changed != 0) { - netif_invoke_ext_callback(netif, LWIP_NSC_IPV4_SETTINGS_CHANGED, NULL); + if (change_reason != LWIP_NSC_NONE) { + change_reason |= LWIP_NSC_IPV4_SETTINGS_CHANGED; + netif_invoke_ext_callback(netif, change_reason, &cb_args); } #endif } diff --git a/src/include/lwip/netif.h b/src/include/lwip/netif.h index bb7362fb..45d0d5e2 100644 --- a/src/include/lwip/netif.h +++ b/src/include/lwip/netif.h @@ -577,24 +577,14 @@ typedef union /** 1: up; 0: down */ u8_t state; } status_changed; - /** Args to LWIP_NSC_IPV4_ADDRESS_CHANGED callback */ + /** Args to LWIP_NSC_IPV4_ADDRESS_CHANGED|LWIP_NSC_IPV4_GATEWAY_CHANGED|LWIP_NSC_IPV4_NETMASK_CHANGED|LWIP_NSC_IPV4_SETTINGS_CHANGED callback */ struct ipv4_changed_s { /** Old IPv4 address */ const ip_addr_t* old_address; + const ip_addr_t* old_netmask; + const ip_addr_t* old_gw; } ipv4_changed; - /** Args to LWIP_NSC_IPV4_GATEWAY_CHANGED callback */ - struct ipv4_gw_changed_s - { - /** Old IPv4 gateway */ - const ip_addr_t* old_address; - } ipv4_gw_changed; - /** Args to LWIP_NSC_IPV4_NETMASK_CHANGED callback */ - struct ipv4_nm_changed_s - { - /** Old IPv4 netmask */ - const ip_addr_t* old_address; - } ipv4_nm_changed; /** Args to LWIP_NSC_IPV6_SET callback */ struct ipv6_set_s { diff --git a/test/unit/core/test_netif.c b/test/unit/core/test_netif.c index fabc49ec..a385b5df 100644 --- a/test/unit/core/test_netif.c +++ b/test/unit/core/test_netif.c @@ -58,10 +58,7 @@ testif_init(struct netif *netif) } #define MAX_NSC_REASON_IDX 10 -static int ext_cb_counters[MAX_NSC_REASON_IDX]; -static netif_nsc_reason_t reasons; - -static netif_ext_callback_args_t *expected_args; +static netif_nsc_reason_t expected_reasons; static int dummy_active; @@ -78,46 +75,11 @@ test_netif_ext_callback_dummy(struct netif* netif, netif_nsc_reason_t reason, co static void test_netif_ext_callback(struct netif* netif, netif_nsc_reason_t reason, const netif_ext_callback_args_t* args) { - int i; - u32_t reason_flags = (u32_t)reason; - u32_t mask; - - reasons = reasons | reason; + LWIP_UNUSED_ARG(args); /* @todo */ fail_unless(netif == &net_test); - fail_unless(reason != 0); - fail_unless((((u32_t)reason) & ~((1U << MAX_NSC_REASON_IDX) - 1U)) == 0); - LWIP_UNUSED_ARG(args); - - for (i = 0, mask = 1U; i < MAX_NSC_REASON_IDX; i++, mask <<= 1) { - if (reason_flags & mask) { - ext_cb_counters[i]++; - } - } - if (expected_args != NULL) { - fail_unless(memcmp(expected_args, args, sizeof(netif_ext_callback_args_t)) == 0); - } -} - -static void -test_netif_ext_callback_assert_flag_count(netif_nsc_reason_t reason, int expected_count) -{ - int i; - u32_t reason_flags = (u32_t)reason; - u32_t mask; - for (i = 0, mask = 1U; i < MAX_NSC_REASON_IDX; i++, mask <<= 1) { - if (reason_flags & mask) { - fail_unless(ext_cb_counters[i] == expected_count); - } - } -} - -static void -test_netif_ext_callback_reset(void) -{ - memset(ext_cb_counters, 0, sizeof(ext_cb_counters)); - reasons = 0; + fail_unless(expected_reasons == reason); } /* Test functions */ @@ -143,32 +105,63 @@ START_TEST(test_netif_extcallbacks) dummy_active = 1; - reasons = 0; - netif_add(&net_test, &addr, &netmask, &gw, &net_test, testif_init, ethernet_input); - fail_unless(reasons == LWIP_NSC_NETIF_ADDED); - test_netif_ext_callback_assert_flag_count(LWIP_NSC_NETIF_ADDED, 1); - test_netif_ext_callback_reset(); + /* positive tests: check that single events come as expected */ + expected_reasons = LWIP_NSC_NETIF_ADDED; + netif_add(&net_test, &addr, &netmask, &gw, &net_test, testif_init, ethernet_input); + + expected_reasons = LWIP_NSC_LINK_CHANGED; netif_set_link_up(&net_test); - fail_unless(reasons == LWIP_NSC_LINK_CHANGED); - test_netif_ext_callback_assert_flag_count(LWIP_NSC_LINK_CHANGED, 1); - test_netif_ext_callback_reset(); + + expected_reasons = LWIP_NSC_STATUS_CHANGED; netif_set_up(&net_test); - fail_unless(reasons == LWIP_NSC_STATUS_CHANGED); - test_netif_ext_callback_assert_flag_count(LWIP_NSC_STATUS_CHANGED, 1); - test_netif_ext_callback_reset(); IP4_ADDR(&addr, 1, 2, 3, 4); + expected_reasons = LWIP_NSC_IPV4_ADDRESS_CHANGED; netif_set_ipaddr(&net_test, &addr); - fail_unless(reasons == LWIP_NSC_IPV4_ADDRESS_CHANGED); - test_netif_ext_callback_assert_flag_count(LWIP_NSC_IPV4_ADDRESS_CHANGED, 1); - test_netif_ext_callback_reset(); + IP4_ADDR(&netmask, 255, 255, 255, 0); + expected_reasons = LWIP_NSC_IPV4_NETMASK_CHANGED; + netif_set_netmask(&net_test, &netmask); + + IP4_ADDR(&gw, 1, 2, 3, 254); + expected_reasons = LWIP_NSC_IPV4_GATEWAY_CHANGED; + netif_set_gw(&net_test, &gw); + + IP4_ADDR(&addr, 0, 0, 0, 0); + expected_reasons = LWIP_NSC_IPV4_ADDRESS_CHANGED; + netif_set_ipaddr(&net_test, &addr); + + IP4_ADDR(&netmask, 0, 0, 0, 0); + expected_reasons = LWIP_NSC_IPV4_NETMASK_CHANGED; + netif_set_netmask(&net_test, &netmask); + + IP4_ADDR(&gw, 0, 0, 0, 0); + expected_reasons = LWIP_NSC_IPV4_GATEWAY_CHANGED; + netif_set_gw(&net_test, &gw); + + /* check that for multi-events (only one combined callback expected) */ + + IP4_ADDR(&addr, 1, 2, 3, 4); + IP4_ADDR(&netmask, 255, 255, 255, 0); + IP4_ADDR(&gw, 1, 2, 3, 254); + expected_reasons = LWIP_NSC_IPV4_ADDRESS_CHANGED | LWIP_NSC_IPV4_NETMASK_CHANGED | LWIP_NSC_IPV4_GATEWAY_CHANGED | LWIP_NSC_IPV4_SETTINGS_CHANGED; + netif_set_addr(&net_test, &addr, &netmask, &gw); + + /* check that for no-change, no callback is expected */ + expected_reasons = 0; + netif_set_ipaddr(&net_test, &addr); + netif_set_netmask(&net_test, &netmask); + netif_set_gw(&net_test, &gw); + netif_set_addr(&net_test, &addr, &netmask, &gw); + + expected_reasons = LWIP_NSC_STATUS_CHANGED; + netif_set_down(&net_test); + + expected_reasons = LWIP_NSC_NETIF_REMOVED; netif_remove(&net_test); - fail_unless(reasons == (LWIP_NSC_NETIF_REMOVED | LWIP_NSC_STATUS_CHANGED)); - test_netif_ext_callback_assert_flag_count(LWIP_NSC_NETIF_REMOVED, 1); - test_netif_ext_callback_assert_flag_count(LWIP_NSC_STATUS_CHANGED, 1); - test_netif_ext_callback_reset(); + + expected_reasons = 0; netif_remove_ext_callback(&netif_callback_2); netif_remove_ext_callback(&netif_callback_3);