Refactor the ARP code to be similar to the dhcp code -- timeout functions and

packet response functions are handled by an array of function pointers indexed
by ARP state.

Split arp_retransmit() apart into simpler functions.

Fix a typo in renewing_timeout() that would result in too-short timeouts that
would soak cpu.

Call handle_arp_timeout() from the timeout_action() function rather than
having
explicit hooks in various <dhcpstate>_timeout() functions.

Make the function pointer arrays static const.

ndhc/arp.c

@@ -1,5 +1,5 @@
 /* arp.c - arp ping checking
- * Time-stamp: <2011-07-06 11:39:23 njk>
+ * Time-stamp: <2011-07-09 17:34:29 njk>
  *
  * Copyright 2010-2011 Nicholas J. Kain <njkain@gmail.com>
  *
@@ -524,8 +524,7 @@ static int arp_gen_probe_wait(void)
     return PROBE_MIN + rand() % (PROBE_MAX - PROBE_MIN);
 }
 
-// Perform retransmission if necessary.
+static void arp_defense_timeout(struct client_state_t *cs)
-void arp_retransmit(struct client_state_t *cs)
 {
     if (pending_def_ts) {
         log_line("arp: Defending our lease IP.");
@@ -537,42 +536,48 @@ void arp_retransmit(struct client_state_t *cs)
             def_old_oldTimeout = 0;
         }
     }
-    if (arpState == AS_GW_CHECK || arpState == AS_GW_QUERY) {
+}
-        if (arpState == AS_GW_CHECK && act_if_arp_gw_failed(cs))
+
-            return;
+static void arp_check_timeout(struct client_state_t *cs)
-        long long cms = curms();
+{
-        long long rtts = arp_send_stats[ASEND_GW_PING].ts + ARP_RETRANS_DELAY;
+    arp_defense_timeout(cs);
-        if (cms < rtts) {
+
-            cs->timeout = rtts - cms;
+    if (arpState == AS_GW_CHECK && act_if_arp_gw_failed(cs))
-            return;
+        return;
-        }
+    long long cms = curms();
-        log_line(arpState == AS_GW_CHECK ?
+    long long rtts = arp_send_stats[ASEND_GW_PING].ts + ARP_RETRANS_DELAY;
-                 "arp: Still waiting for gateway to reply to arp ping..." :
+    if (cms < rtts) {
-                 "arp: Still looking for gateway hardware address...");
+        cs->timeout = rtts - cms;
-        if (arp_ping(cs, cs->routerAddr) == -1)
-            log_warning("arp: Failed to send ARP ping in retransmission.");
-        cs->timeout = ARP_RETRANS_DELAY;
         return;
     }
-    if (arpState == AS_COLLISION_CHECK) {
+    log_line(arpState == AS_GW_CHECK ?
-        long long cms = curms();
+             "arp: Still waiting for gateway to reply to arp ping..." :
-        if (cms >= collision_check_init_ts + ANNOUNCE_WAIT ||
+             "arp: Still looking for gateway hardware address...");
-            arp_send_stats[ASEND_COLLISION_CHECK].count >= PROBE_NUM) {
+    if (arp_ping(cs, cs->routerAddr) == -1)
-            arp_success(cs);
+        log_warning("arp: Failed to send ARP ping in retransmission.");
-            return;
+    cs->timeout = ARP_RETRANS_DELAY;
-        }
+}
-        long long rtts = arp_send_stats[ASEND_COLLISION_CHECK].ts +
+
-            probe_wait_time;
+static void arp_collision_timeout(struct client_state_t *cs)
-        if (cms < rtts) {
+{
-            cs->timeout = rtts - cms;
+    arp_defense_timeout(cs);
-            return;
+
-        }
+    long long cms = curms();
-        log_line("arp: Probing for hosts that may conflict with our lease...");
+    if (cms >= collision_check_init_ts + ANNOUNCE_WAIT ||
-        if (arp_ip_anon_ping(cs, arp_dhcp_packet.yiaddr) == -1)
+        arp_send_stats[ASEND_COLLISION_CHECK].count >= PROBE_NUM) {
-            log_warning("arp: Failed to send ARP ping in retransmission.");
+        arp_success(cs);
-        cs->timeout = probe_wait_time = arp_gen_probe_wait();
         return;
     }
+    long long rtts = arp_send_stats[ASEND_COLLISION_CHECK].ts +
+        probe_wait_time;
+    if (cms < rtts) {
+        cs->timeout = rtts - cms;
+        return;
+    }
+    log_line("arp: Probing for hosts that may conflict with our lease...");
+    if (arp_ip_anon_ping(cs, arp_dhcp_packet.yiaddr) == -1)
+        log_warning("arp: Failed to send ARP ping in retransmission.");
+    cs->timeout = probe_wait_time = arp_gen_probe_wait();
 }
 
 static void arp_do_defense(struct client_state_t *cs)
@@ -600,6 +605,72 @@ static void arp_do_defense(struct client_state_t *cs)
     last_conflict_ts = cur_conflict_ts;
 }
 
+static void arp_do_gw_query(struct client_state_t *cs)
+{
+    if (arp_is_query_reply(&arpreply) &&
+        !memcmp(arpreply.sip4, &cs->routerAddr, 4)) {
+        cs->timeout = cs->oldTimeout;
+        memcpy(cs->routerArp, arpreply.smac, 6);
+        log_line("arp: Gateway hardware address %02x:%02x:%02x:%02x:%02x:%02x",
+                 cs->routerArp[0], cs->routerArp[1],
+                 cs->routerArp[2], cs->routerArp[3],
+                 cs->routerArp[4], cs->routerArp[5]);
+        arp_switch_state(cs, AS_DEFENSE);
+        arp_announcement(cs);  // Do a second announcement.
+        return;
+    }
+    if (!arp_validate_bpf_defense(cs, &arpreply))
+        return;
+    arp_do_defense(cs);
+}
+
+static void arp_do_collision_check(struct client_state_t *cs)
+{
+    if (!arp_is_query_reply(&arpreply))
+        return;
+    // If this packet was sent from our lease IP, and does not have a
+    // MAC address matching our own (the latter check guards against stupid
+    // hubs or repeaters), then it's a conflict and thus a failure.
+    if (!memcmp(arpreply.sip4, &arp_dhcp_packet.yiaddr, 4) &&
+        !memcmp(client_config.arp, arpreply.smac, 6)) {
+        total_conflicts++;
+        arp_failed(cs);
+    }
+}
+
+static void arp_do_gw_check(struct client_state_t *cs)
+{
+    if (!arp_is_query_reply(&arpreply))
+        return;
+    if (!memcmp(arpreply.sip4, &cs->routerAddr, 4)) {
+        // Success only if the router/gw MAC matches stored value
+        if (!memcmp(cs->routerArp, arpreply.smac, 6))
+            arp_gw_success(cs);
+        else
+            arp_gw_failed(cs);
+    }
+}
+
+static void arp_do_invalid(struct client_state_t *cs)
+{
+    log_error("handle_arp_response: called in invalid state %u", arpState);
+    arp_close_fd(cs);
+}
+
+typedef struct {
+    void (*packet_fn)(struct client_state_t *cs);
+    void (*timeout_fn)(struct client_state_t *cs);
+} arp_state_fn_t;
+
+static const arp_state_fn_t arp_states[] = {
+    { arp_do_invalid, 0 }, // AS_NONE
+    { arp_do_collision_check, arp_collision_timeout }, // AS_COLLISION_CHECK
+    { arp_do_gw_check, arp_check_timeout }, // AS_GW_CHECK
+    { arp_do_gw_query, arp_check_timeout }, // AS_GW_QUERY
+    { arp_do_defense, arp_defense_timeout }, // AS_DEFENSE
+    { arp_do_invalid, 0 }, // AS_MAX
+};
+
 void handle_arp_response(struct client_state_t *cs)
 {
     int r = 0;
@@ -620,7 +691,7 @@ void handle_arp_response(struct client_state_t *cs)
     }
 
     if (r <= 0) {
-        arp_retransmit(cs);
+        handle_arp_timeout(cs);
         return;
     }
 
@@ -635,51 +706,14 @@ void handle_arp_response(struct client_state_t *cs)
         return;
     }
 
-    switch (arpState) {
+    if (arp_states[arpState].packet_fn)
-    case AS_COLLISION_CHECK:
+        arp_states[arpState].packet_fn(cs);
-        if (!arp_is_query_reply(&arpreply))
-            break;
-        // If this packet was sent from our lease IP, and does not have a
-        // MAC address matching our own (the latter check guards against stupid
-        // hubs or repeaters), then it's a conflict and thus a failure.
-        if (!memcmp(arpreply.sip4, &arp_dhcp_packet.yiaddr, 4) &&
-            !memcmp(client_config.arp, arpreply.smac, 6)) {
-            total_conflicts++;
-            arp_failed(cs);
-        }
-        break;
-    case AS_GW_CHECK:
-        if (!arp_is_query_reply(&arpreply))
-            break;
-        if (!memcmp(arpreply.sip4, &cs->routerAddr, 4)) {
-            // Success only if the router/gw MAC matches stored value
-            if (!memcmp(cs->routerArp, arpreply.smac, 6))
-                arp_gw_success(cs);
-            else
-                arp_gw_failed(cs);
-        }
-        break;
-    case AS_GW_QUERY:
-        if (arp_is_query_reply(&arpreply) &&
-            !memcmp(arpreply.sip4, &cs->routerAddr, 4)) {
-            cs->timeout = cs->oldTimeout;
-            memcpy(cs->routerArp, arpreply.smac, 6);
-            log_line("arp: Gateway hardware address %02x:%02x:%02x:%02x:%02x:%02x",
-                     cs->routerArp[0], cs->routerArp[1],
-                     cs->routerArp[2], cs->routerArp[3],
-                     cs->routerArp[4], cs->routerArp[5]);
-            arp_switch_state(cs, AS_DEFENSE);
-            arp_announcement(cs);  // Do a second announcement.
-            break;
-        }
-        if (!arp_validate_bpf_defense(cs, &arpreply))
-            break;
-    case AS_DEFENSE:
-        arp_do_defense(cs);
-        break;
-    default:
-        log_error("handle_arp_response: called in invalid state %u", arpState);
-        arp_close_fd(cs);
-    }
     arpreply_clear();
 }
+
+// Perform retransmission if necessary.
+void handle_arp_timeout(struct client_state_t *cs)
+{
+    if (arp_states[arpState].timeout_fn)
+        arp_states[arpState].timeout_fn(cs);
+}

ndhc/arp.h

@@ -1,5 +1,5 @@
 /* arp.h - functions to call the interface change daemon
- * Time-stamp: <2011-07-06 11:39:42 njk>
+ * Time-stamp: <2011-07-09 15:39:46 njk>
  *
  * Copyright 2010-2011 Nicholas J. Kain <njkain@gmail.com>
  *
@@ -53,7 +53,7 @@ int arp_check(struct client_state_t *cs, struct dhcpmsg *packet);
 int arp_gw_check(struct client_state_t *cs);
 void arp_set_defense_mode(struct client_state_t *cs);
 void arp_success(struct client_state_t *cs);
-void arp_retransmit(struct client_state_t *cs);
 void handle_arp_response(struct client_state_t *cs);
+void handle_arp_timeout(struct client_state_t *cs);
 
 #endif /* ARP_H_ */

ndhc/state.c

@@ -22,8 +22,6 @@ static void bound_timeout(struct client_state_t *cs);
 static void renewing_timeout(struct client_state_t *cs);
 static void rebinding_timeout(struct client_state_t *cs);
 static void released_timeout(struct client_state_t *cs);
-static void collision_check_timeout(struct client_state_t *cs);
-static void bound_gw_check_timeout(struct client_state_t *cs);
 static void xmit_release(struct client_state_t *cs);
 static void print_release(struct client_state_t *cs);
 static void frenew(struct client_state_t *cs);
@@ -36,14 +34,14 @@ typedef struct {
     void (*force_release_fn)(struct client_state_t *cs);
 } dhcp_state_t;
 
-dhcp_state_t dhcp_states[] = {
+static const dhcp_state_t dhcp_states[] = {
     { selecting_packet, selecting_timeout, 0, print_release}, // SELECTING
     { an_packet, requesting_timeout, 0, print_release},       // REQUESTING
     { 0, bound_timeout, frenew, xmit_release},                // BOUND
     { an_packet, renewing_timeout, 0, xmit_release},          // RENEWING
     { an_packet, rebinding_timeout, 0, xmit_release},         // REBINDING
-    { 0, bound_gw_check_timeout, 0, xmit_release},            // BOUND_GW_CHECK
+    { 0, 0, 0, xmit_release},                                 // BOUND_GW_CHECK
-    { 0, collision_check_timeout, 0, xmit_release},          // COLLISION_CHECK
+    { 0, 0, 0, xmit_release},                                // COLLISION_CHECK
     { 0, released_timeout, frenew, 0},                       // RELEASED
     { 0, 0, 0, 0},                                           // NUM_STATES
 };
@@ -101,8 +99,6 @@ static void requesting_timeout(struct client_state_t *cs)
 // total time, and it is time to renew the lease so that it is not lost.
 static void bound_timeout(struct client_state_t *cs)
 {
-    arp_retransmit(cs);
-
     if (curms() < cs->leaseStartTime + cs->renewTime * 1000)
         return;
     cs->dhcpState = DS_RENEWING;
@@ -139,7 +135,7 @@ static void renewing_timeout(struct client_state_t *cs)
     long long elt = cs->leaseStartTime + cs->lease * 1000;
     if (ct < elt) {
         cs->dhcpState = DS_REBINDING;
-        cs->timeout = elt - ct / 2;
+        cs->timeout = (elt - ct) / 2;
         log_line("Entering rebinding state.");
     } else
         lease_timedout(cs);
@@ -169,16 +165,6 @@ static void released_timeout(struct client_state_t *cs)
     cs->timeout = -1;
 }
 
-static void collision_check_timeout(struct client_state_t *cs)
-{
-    arp_retransmit(cs);
-}
-
-static void bound_gw_check_timeout(struct client_state_t *cs)
-{
-    arp_retransmit(cs);
-}
-
 // Can transition to DS_BOUND or DS_SELECTING.
 static void an_packet(struct client_state_t *cs, struct dhcpmsg *packet,
                       uint8_t *message)
@@ -332,6 +318,7 @@ void packet_action(struct client_state_t *cs, struct dhcpmsg *packet,
 
 void timeout_action(struct client_state_t *cs)
 {
+    handle_arp_timeout(cs);
     if (dhcp_states[cs->dhcpState].timeout_fn)
         dhcp_states[cs->dhcpState].timeout_fn(cs);
 }