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

@@ -421,28 +421,29 @@ netif_add(struct netif *netif,
 }
 
 #if LWIP_IPV4
-static void
+static int
-netif_do_set_ipaddr(struct netif *netif, const ip4_addr_t *ipaddr, u8_t callback)
+netif_do_set_ipaddr(struct netif *netif, const ip4_addr_t *ipaddr, ip_addr_t *old_addr)
 {
-  ip_addr_t new_addr;
+  LWIP_ASSERT("invalid pointer", ipaddr != NULL);
-
+  LWIP_ASSERT("invalid pointer", old_addr != NULL);
-  *ip_2_ip4(&new_addr) = *ipaddr;
-  IP_SET_TYPE_VAL(new_addr, IPADDR_TYPE_V4);
 
   /* address is actually being changed? */
-  if (ip4_addr_cmp(ip_2_ip4(&new_addr), netif_ip4_addr(netif)) == 0) {
+  if (ip4_addr_cmp(ipaddr, netif_ip4_addr(netif)) == 0) {
-    ip_addr_t old_addr;
+    ip_addr_t new_addr;
-    ip_addr_copy(old_addr, *netif_ip_addr4(netif));
+    *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) {
+    return 1; /* address changed */
-#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 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
+static int
-netif_do_set_netmask(struct netif *netif, const ip4_addr_t *netmask, u8_t callback)
+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;
+    LWIP_ASSERT("invalid pointer", old_nm != NULL);
-    ip_addr_t old_addr;
+    ip_addr_copy(*old_nm, *netif_ip_netmask4(netif));
-    ip_addr_copy(old_addr, *netif_ip_netmask4(netif));
+#else
-    args.ipv4_nm_changed.old_address = &old_addr;
+    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))));
-
+    return 1; /* netmask changed */
-    if (callback) {
-      netif_invoke_ext_callback(netif, LWIP_NSC_IPV4_NETMASK_CHANGED, &args);
-    }
   }
+  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
+static int
-netif_do_set_gw(struct netif *netif, const ip4_addr_t *gw, u8_t callback)
+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;
+    LWIP_ASSERT("invalid pointer", old_gw != NULL);
-    ip_addr_t old_addr;
+    ip_addr_copy(*old_gw, *netif_ip_gw4(netif));
-    ip_addr_copy(old_addr, *netif_ip_gw4(netif));
+#else
-    args.ipv4_gw_changed.old_address = &old_addr;
+    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))));
-
+    return 1; /* gateway changed */
-    if (callback) {
-      netif_invoke_ext_callback(netif, LWIP_NSC_IPV4_GATEWAY_CHANGED, &args);
-    }
   }
+  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
+  remove = ip4_addr_isany(ipaddr);
-  if ((ip4_addr_cmp(ipaddr,  netif_ip4_addr(netif))    == 0) ||
+  if (remove) {
-      (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)) {
     /* 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);
+    if (netif_do_set_ipaddr(netif, ipaddr, &old_addr)) {
-    netif_do_set_netmask(netif, netmask, 0);
+#if LWIP_NETIF_EXT_STATUS_CALLBACK
-    netif_do_set_gw(netif, gw, 0);
+      change_reason |= LWIP_NSC_IPV4_ADDRESS_CHANGED;
-  } else {
+      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);
+    if (netif_do_set_ipaddr(netif, ipaddr, &old_addr)) {
-    netif_do_set_gw(netif, gw, 0);
+#if LWIP_NETIF_EXT_STATUS_CALLBACK
-    netif_do_set_ipaddr(netif, ipaddr, 0);
+      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) {
+  if (change_reason != LWIP_NSC_NONE) {
-    netif_invoke_ext_callback(netif, LWIP_NSC_IPV4_SETTINGS_CHANGED, NULL);
+    change_reason |= LWIP_NSC_IPV4_SETTINGS_CHANGED;
+    netif_invoke_ext_callback(netif, change_reason, &cb_args);
   }
 #endif
 }

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
   {

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 expected_reasons;
-static netif_nsc_reason_t reasons;
-
-static netif_ext_callback_args_t *expected_args;
 
 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;
+  LWIP_UNUSED_ARG(args); /* @todo */
-  u32_t reason_flags = (u32_t)reason;
-  u32_t mask;
-
-  reasons = reasons | reason;
 
   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);
+  fail_unless(expected_reasons == reason);
-
-  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;
 }
 
 /* Test functions */
@@ -143,32 +105,63 @@ START_TEST(test_netif_extcallbacks)
 
   dummy_active = 1;
 
-  reasons = 0;
+  /* positive tests: check that single events come as expected */
-  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();
 
+  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);
+  expected_reasons = LWIP_NSC_STATUS_CHANGED;
-  test_netif_ext_callback_reset();
   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);
+  expected_reasons = 0;
-  test_netif_ext_callback_assert_flag_count(LWIP_NSC_STATUS_CHANGED, 1);
-  test_netif_ext_callback_reset();
 
   netif_remove_ext_callback(&netif_callback_2);
   netif_remove_ext_callback(&netif_callback_3);