Remove ifsPrevState and set non-infinite timeout on a send error.

We instead check carrier status as needed.  This approach is more
robust.  For a simple example, imagine link state changes that happen
while the machine is suspended.

src/ndhc.c

@@ -260,8 +260,7 @@ static void fail_if_state_dir_dne(void)
 static void do_ndhc_work(void)
 {
     static bool rfkill_set; // Is the rfkill switch set?
-    static bool rfkill_nl_state_changed; // iface state changed during rfkill
+    static bool rfkill_nl_carrier_wentup; // iface carrier changed to up during rfkill
-    static int rfkill_nl_state; // current iface state during rfkill
     struct dhcpmsg dhcp_packet;
     struct epoll_event events[1];
     long long nowts;
@@ -333,45 +332,33 @@ static void do_ndhc_work(void)
                 sev_rfk = rfkill_get(&cs, 1, client_config.rfkillIdx);
             } else
                 suicide("epoll_wait: unknown fd");
+        }
 
-            if (sev_rfk == RFK_ENABLED) {
+        if (sev_rfk == RFK_ENABLED) {
-                rfkill_set = 1;
+            rfkill_set = 1;
-                rfkill_nl_state = cs.ifsPrevState;
+            rfkill_nl_carrier_wentup = false;
-                rfkill_nl_state_changed = false;
+            log_line("rfkill: radio now blocked");
-                log_line("rfkill: radio now blocked");
+        } else if (sev_rfk == RFK_DISABLED) {
-            } else if (sev_rfk == RFK_DISABLED) {
+            rfkill_set = 0;
-                rfkill_set = 0;
+            log_line("rfkill: radio now unblocked");
-                log_line("rfkill: radio now unblocked");
+            if (rfkill_nl_carrier_wentup && carrier_isup()) {
-                // We now simulate the state changes that may have happened
+                // We might have changed networks while the radio was down.
-                // during rfkill.
+                force_fingerprint = true;
-                if (rfkill_nl_state != cs.ifsPrevState)
-                    nl_event_react(&cs, rfkill_nl_state);
-                else if (rfkill_nl_state_changed && rfkill_nl_state == IFS_UP) {
-                    // We might have changed networks even if we ended up
-                    // back in IFS_UP state.  We need to fingerprint the
-                    // network and confirm that we're on the same network.
-                    force_fingerprint = true;
-                }
             }
         }
 
-        if (sev_nl != IFS_NONE) {
+        if (sev_nl != IFS_NONE && nl_event_carrier_wentup(sev_nl)) {
-            if (!rfkill_set) {
+            if (!rfkill_set)
-                if (nl_event_react(&cs, sev_nl))
+                force_fingerprint = true;
-                    force_fingerprint = true;
+            else
-            } else {
+                rfkill_nl_carrier_wentup = true;
-                // Store the state so it can be replayed later.
-                rfkill_nl_state_changed = true;
-                rfkill_nl_state = sev_nl;
-            }
         }
 
-        if (rfkill_set || cs.ifsPrevState != IFS_UP) {
+        if (rfkill_set || !carrier_isup()) {
             // We can't do anything while the iface is disabled, anyway.
-            // XXX: It may be smart to set a non-infinite timeout
+            // Suspend might cause link state change notifications to be
-            // and periodically poll to see if the rfkill or iface
+            // missed, so we use a non-infinite timeout.
-            // state changed; it might happen during suspend.
+            timeout = 2000 + nk_random_u32(&cs.rnd32_state) % 3000;
-            timeout = -1;
             continue;
         }
 
@@ -385,11 +372,8 @@ static void do_ndhc_work(void)
         if (sev_arp)
             arp_reply_clear();
 
-        // XXX: Would be best if we detected RFKILL being set via an
-        //      error message and propagated it back to here as a
-        //      distinct return value.
         if (dhcp_ok == COR_ERROR) {
-            timeout = -1;
+            timeout = 2000 + nk_random_u32(&cs.rnd32_state) % 3000;
             continue;
         }
 
@@ -500,7 +484,7 @@ static void ndhc_main(void) {
     memset(chroot_dir, '\0', sizeof chroot_dir);
     nk_set_uidgid(ndhc_uid, ndhc_gid, NULL, 0);
 
-    if (cs.ifsPrevState != IFS_UP) {
+    if (!carrier_isup()) {
         if (ifchange_deconfig(&cs) < 0)
             suicide("%s: can't deconfigure interface settings", __func__);
     }
@@ -584,8 +568,7 @@ int main(int argc, char *argv[])
     get_clientid(&cs, &client_config);
 
     switch (perform_ifup()) {
-    case 1: cs.ifsPrevState = IFS_UP;
+    case 1: case 0: break;
-    case 0: break;
     case -3: wait_for_rfkill(); break;
     default: suicide("failed to set the interface to up state");
     }

src/ndhc.h

@@ -38,7 +38,6 @@ struct client_state_t {
     struct nk_random_state_u32 rnd32_state;
     long long leaseStartTime, renewTime, rebindTime;
     long long dhcp_wake_ts;
-    int ifsPrevState;
     int ifDeconfig; // Set if the interface has already been deconfigured.
     int epollFd, signalFd, listenFd, arpFd, nlFd, rfkillFd;
     int nlPortId;

src/netlink.c

@@ -43,35 +43,25 @@
 #include "nl.h"
 #include "state.h"
 
-int nl_event_react(struct client_state_t cs[static 1], int state)
+// Returns true if the current interface state is UP.
+bool nl_event_carrier_wentup(int state)
 {
-    if (state == cs->ifsPrevState)
-        return 0;
-
     switch (state) {
     case IFS_UP:
-        cs->ifsPrevState = IFS_UP;
+        log_line("%s: Carrier up.", client_config.interface);
-        return 1;
+        return true;
     case IFS_DOWN:
         // Interface configured, but no hardware carrier.
-        cs->ifsPrevState = IFS_DOWN;
         log_line("%s: Carrier down.", client_config.interface);
-        return 0;
+        return false;
     case IFS_SHUT:
         // User shut down the interface.
-        cs->ifsPrevState = IFS_SHUT;
+        log_line("%s: Interface shut down.", client_config.interface);
-        log_line("%s: Interface shut down.  Going to sleep.",
+        return false;
-                 client_config.interface);
-        // XXX: I think this was wrong; instead it should just sleep.
-        //      The lease has not expired just because the user shut down
-        //      the interface...
-        // set_released(cs);
-        return 0;
     case IFS_REMOVED:
-        cs->ifsPrevState = IFS_REMOVED;
         log_line("Interface removed.  Exiting.");
         exit(EXIT_SUCCESS);
-    default: return 0;
+    default: return false;
     }
 }
 

src/netlink.h

@@ -29,6 +29,7 @@
 #ifndef NK_NETLINK_H_
 #define NK_NETLINK_H_
 
+#include <stdbool.h>
 #include <linux/rtnetlink.h>
 #include "state.h"
 
@@ -40,7 +41,7 @@ enum {
     IFS_REMOVED
 };
 
-int nl_event_react(struct client_state_t cs[static 1], int state);
+bool nl_event_carrier_wentup(int state);
 int nl_event_get(struct client_state_t cs[static 1]);
 int nl_getifdata(void);