zcip: use low-order 4 bytes of MAC as random seed, not 4 high-order
authorDenis Vlasenko <vda.linux@googlemail.com>
Sun, 25 Nov 2007 12:40:56 +0000 (12:40 -0000)
committerDenis Vlasenko <vda.linux@googlemail.com>
Sun, 25 Nov 2007 12:40:56 +0000 (12:40 -0000)
arping: fix wrong roundtrip calculation
arping,zcip: cleanups and code shrink

run                                                  389     402     +13
arp                                                  195     188      -7
zcip_main                                           1524    1495     -29
arping_main                                         1874    1823     -51
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 1/3 up/down: 13/-87)            Total: -74 bytes
   text    data     bss     dec     hex filename
 776587     929    9100  786616   c00b8 busybox_old
 776499     929    9100  786528   c0060 busybox_unstripped

networking/arping.c
networking/zcip.c

index 771c3bc..6383f9a 100644 (file)
@@ -33,7 +33,7 @@ struct globals {
        struct in_addr dst;
        struct sockaddr_ll me;
        struct sockaddr_ll he;
-       int sock;
+       int sock_fd;
 
        int count; // = -1;
        unsigned last;
@@ -51,7 +51,7 @@ struct globals {
 #define dst        (G.dst       )
 #define me         (G.me        )
 #define he         (G.he        )
-#define sock       (G.sock      )
+#define sock_fd    (G.sock_fd   )
 #define count      (G.count     )
 #define last       (G.last      )
 #define timeout_us (G.timeout_us)
@@ -71,7 +71,6 @@ static int send_pack(struct in_addr *src_addr,
                        struct sockaddr_ll *HE)
 {
        int err;
-       unsigned now;
        unsigned char buf[256];
        struct arphdr *ah = (struct arphdr *) buf;
        unsigned char *p = (unsigned char *) (ah + 1);
@@ -98,10 +97,9 @@ static int send_pack(struct in_addr *src_addr,
        memcpy(p, dst_addr, 4);
        p += 4;
 
-       now = MONOTONIC_US();
-       err = sendto(sock, buf, p - buf, 0, (struct sockaddr *) HE, sizeof(*HE));
+       err = sendto(sock_fd, buf, p - buf, 0, (struct sockaddr *) HE, sizeof(*HE));
        if (err == p - buf) {
-               last = now;
+               last = MONOTONIC_US();
                sent++;
                if (!(option_mask32 & UNICASTING))
                        brd_sent++;
@@ -135,7 +133,7 @@ static void catcher(void)
        if (start == 0)
                start = now;
 
-       if (count == 0 || (timeout_us && (now - start) > (timeout_us + 500000)))
+       if (count == 0 || (timeout_us && (now - start) > timeout_us))
                finish();
 
        /* count < 0 means "infinite count" */
@@ -183,7 +181,7 @@ static int recv_pack(unsigned char *buf, int len, struct sockaddr_ll *FROM)
        memcpy(&src_ip, p + ah->ar_hln, 4);
        memcpy(&dst_ip, p + ah->ar_hln + 4 + ah->ar_hln, 4);
        if (!(option_mask32 & DAD)) {
-               if (src_ip.s_addr != dst.s_addr)
+               if (dst.s_addr != src_ip.s_addr)
                        return 0;
                if (src.s_addr != dst_ip.s_addr)
                        return 0;
@@ -230,7 +228,8 @@ static int recv_pack(unsigned char *buf, int len, struct sockaddr_ll *FROM)
                }
 
                if (last) {
-                       printf(" %u.%03ums\n", last / 1000, last % 1000);
+                       unsigned diff = MONOTONIC_US() - last;
+                       printf(" %u.%03ums\n", diff / 1000, diff % 1000);
                } else {
                        printf(" UNSOLICITED?\n");
                }
@@ -254,17 +253,17 @@ int arping_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
 int arping_main(int argc, char **argv)
 {
        const char *device = "eth0";
-       int ifindex;
        char *source = NULL;
        char *target;
        unsigned char *packet;
 
        INIT_G();
 
-       sock = xsocket(PF_PACKET, SOCK_DGRAM, 0);
+       sock_fd = xsocket(AF_PACKET, SOCK_DGRAM, 0);
 
        // Drop suid root privileges
-       xsetuid(getuid());
+       // Need to remove SUID_NEVER from applets.h for this to work
+       //xsetuid(getuid());
 
        {
                unsigned opt;
@@ -279,7 +278,7 @@ int arping_main(int argc, char **argv)
                if (opt & 0x40) /* -c: count */
                        count = xatoi_u(str_count);
                if (opt & 0x80) /* -w: timeout */
-                       timeout_us = xatou_range(str_timeout, 0, INT_MAX/2000000) * 1000000;
+                       timeout_us = xatou_range(str_timeout, 0, INT_MAX/2000000) * 1000000 + 500000;
                //if (opt & 0x200) /* -s: source */
                option_mask32 &= 0x3f; /* set respective flags */
        }
@@ -295,10 +294,10 @@ int arping_main(int argc, char **argv)
                strncpy(ifr.ifr_name, device, sizeof(ifr.ifr_name) - 1);
                /* We use ifr.ifr_name in error msg so that problem
                 * with truncated name will be visible */
-               ioctl_or_perror_and_die(sock, SIOCGIFINDEX, &ifr, "interface %s not found", ifr.ifr_name);
-               ifindex = ifr.ifr_ifindex;
+               ioctl_or_perror_and_die(sock_fd, SIOCGIFINDEX, &ifr, "interface %s not found", ifr.ifr_name);
+               me.sll_ifindex = ifr.ifr_ifindex;
 
-               xioctl(sock, SIOCGIFFLAGS, (char *) &ifr);
+               xioctl(sock_fd, SIOCGIFFLAGS, (char *) &ifr);
 
                if (!(ifr.ifr_flags & IFF_UP)) {
                        bb_error_msg_and_die("interface %s is down", device);
@@ -309,7 +308,7 @@ int arping_main(int argc, char **argv)
                }
        }
 
-       if (!inet_aton(target, &dst)) {
+       /* if (!inet_aton(target, &dst)) - not needed */ {
                len_and_sockaddr *lsa;
                lsa = xhost_and_af2sockaddr(target, 0, AF_INET);
                memcpy(&dst, &lsa->sin.sin_addr.s_addr, 4);
@@ -321,23 +320,23 @@ int arping_main(int argc, char **argv)
                bb_error_msg_and_die("invalid source address %s", source);
        }
 
-       if (!(option_mask32 & DAD) && (option_mask32 & UNSOLICITED) && src.s_addr == 0)
+       if ((option_mask32 & (DAD|UNSOLICITED)) == UNSOLICITED && src.s_addr == 0)
                src = dst;
 
        if (!(option_mask32 & DAD) || src.s_addr) {
                struct sockaddr_in saddr;
                int probe_fd = xsocket(AF_INET, SOCK_DGRAM, 0);
 
-               if (device) {
-                       if (setsockopt(probe_fd, SOL_SOCKET, SO_BINDTODEVICE, device, strlen(device) + 1) == -1)
-                               bb_perror_msg("cannot bind to device %s", device);
-               }
+               if (setsockopt(probe_fd, SOL_SOCKET, SO_BINDTODEVICE, device, strlen(device) + 1) == -1)
+                       bb_perror_msg("cannot bind to device %s", device);
                memset(&saddr, 0, sizeof(saddr));
                saddr.sin_family = AF_INET;
                if (src.s_addr) {
+                       /* Check that this is indeed our IP */
                        saddr.sin_addr = src;
                        xbind(probe_fd, (struct sockaddr *) &saddr, sizeof(saddr));
-               } else if (!(option_mask32 & DAD)) {
+               } else { /* !(option_mask32 & DAD) case */
+                       /* Find IP address on this iface */
                        socklen_t alen = sizeof(saddr);
 
                        saddr.sin_port = htons(1025);
@@ -347,23 +346,25 @@ int arping_main(int argc, char **argv)
                                bb_perror_msg("setsockopt(SO_DONTROUTE)");
                        xconnect(probe_fd, (struct sockaddr *) &saddr, sizeof(saddr));
                        if (getsockname(probe_fd, (struct sockaddr *) &saddr, &alen) == -1) {
-                               bb_error_msg_and_die("getsockname");
+                               bb_perror_msg_and_die("getsockname");
                        }
+                       if (saddr.sin_family != AF_INET)
+                               bb_error_msg_and_die("no IP address configured");
                        src = saddr.sin_addr;
                }
                close(probe_fd);
        }
 
        me.sll_family = AF_PACKET;
-       me.sll_ifindex = ifindex;
+       //me.sll_ifindex = ifindex; - done before
        me.sll_protocol = htons(ETH_P_ARP);
-       xbind(sock, (struct sockaddr *) &me, sizeof(me));
+       xbind(sock_fd, (struct sockaddr *) &me, sizeof(me));
 
        {
                socklen_t alen = sizeof(me);
 
-               if (getsockname(sock, (struct sockaddr *) &me, &alen) == -1) {
-                       bb_error_msg_and_die("getsockname");
+               if (getsockname(sock_fd, (struct sockaddr *) &me, &alen) == -1) {
+                       bb_perror_msg_and_die("getsockname");
                }
        }
        if (me.sll_halen == 0) {
@@ -373,14 +374,10 @@ int arping_main(int argc, char **argv)
        he = me;
        memset(he.sll_addr, -1, he.sll_halen);
 
-       if (!src.s_addr && !(option_mask32 & DAD)) {
-               bb_error_msg_and_die("no src address in the non-DAD mode");
-       }
-
        if (!(option_mask32 & QUIET)) {
-               printf("ARPING to %s from %s via %s\n",
-                       inet_ntoa(dst), inet_ntoa(src),
-                       device ? device : "unknown");
+               /* inet_ntoa uses static storage, can't use in same printf */
+               printf("ARPING to %s", inet_ntoa(dst));
+               printf(" from %s via %s\n", inet_ntoa(src), device);
        }
 
        {
@@ -405,7 +402,7 @@ int arping_main(int argc, char **argv)
                socklen_t alen = sizeof(from);
                int cc;
 
-               cc = recvfrom(sock, packet, 4096, 0, (struct sockaddr *) &from, &alen);
+               cc = recvfrom(sock_fd, packet, 4096, 0, (struct sockaddr *) &from, &alen);
                if (cc < 0) {
                        bb_perror_msg("recvfrom");
                        continue;
index a16a642..129155f 100644 (file)
@@ -39,7 +39,7 @@
 #define MONOTONIC_US() ((unsigned)monotonic_us())
 
 struct arp_packet {
-       struct ether_header hdr;
+       struct ether_header eth;
        struct ether_arp arp;
 } ATTRIBUTE_PACKED;
 
@@ -69,8 +69,21 @@ enum {
        DEFEND
 };
 
-#define VDBG(fmt,args...) \
-       do { } while (0)
+#define VDBG(...) do { } while (0)
+
+
+enum {
+       sock_fd = 3
+};
+
+struct globals {
+       char *intf;
+       struct sockaddr saddr;
+};
+#define G (*(struct globals*)&bb_common_bufsiz1)
+#define intf  (G.intf )
+#define saddr (G.saddr)
+
 
 /**
  * Pick a random link local IP address on 169.254/16, except that
@@ -89,17 +102,17 @@ static void pick(struct in_addr *ip)
 /**
  * Broadcast an ARP packet.
  */
-static void arp(int fd, struct sockaddr *saddr, int op,
-       const struct ether_addr *source_addr, struct in_addr source_ip,
-       const struct ether_addr *target_addr, struct in_addr target_ip)
+static void arp(int op,
+       const struct ether_addr *source_eth, struct in_addr source_ip,
+       const struct ether_addr *target_eth, struct in_addr target_ip)
 {
        struct arp_packet p;
        memset(&p, 0, sizeof(p));
 
        // ether header
-       p.hdr.ether_type = htons(ETHERTYPE_ARP);
-       memcpy(p.hdr.ether_shost, source_addr, ETH_ALEN);
-       memset(p.hdr.ether_dhost, 0xff, ETH_ALEN);
+       p.eth.ether_type = htons(ETHERTYPE_ARP);
+       memcpy(p.eth.ether_shost, source_eth, ETH_ALEN);
+       memset(p.eth.ether_dhost, 0xff, ETH_ALEN);
 
        // arp request
        p.arp.arp_hrd = htons(ARPHRD_ETHER);
@@ -107,13 +120,13 @@ static void arp(int fd, struct sockaddr *saddr, int op,
        p.arp.arp_hln = ETH_ALEN;
        p.arp.arp_pln = 4;
        p.arp.arp_op = htons(op);
-       memcpy(&p.arp.arp_sha, source_addr, ETH_ALEN);
+       memcpy(&p.arp.arp_sha, source_eth, ETH_ALEN);
        memcpy(&p.arp.arp_spa, &source_ip, sizeof(p.arp.arp_spa));
-       memcpy(&p.arp.arp_tha, target_addr, ETH_ALEN);
+       memcpy(&p.arp.arp_tha, target_eth, ETH_ALEN);
        memcpy(&p.arp.arp_tpa, &target_ip, sizeof(p.arp.arp_tpa));
 
        // send it
-       xsendto(fd, &p, sizeof(p), saddr, sizeof(*saddr));
+       xsendto(sock_fd, &p, sizeof(p), &saddr, sizeof(saddr));
 
        // Currently all callers ignore errors, that's why returns are
        // commented out...
@@ -123,21 +136,24 @@ static void arp(int fd, struct sockaddr *saddr, int op,
 /**
  * Run a script. argv[2] is already NULL.
  */
-static int run(char *argv[3], const char *intf, struct in_addr *ip)
+static int run(char *argv[3], struct in_addr *ip)
 {
        int status;
+       char *addr = addr; /* for gcc */
+       const char *fmt = "%s %s %s" + 3;
 
        VDBG("%s run %s %s\n", intf, argv[0], argv[1]);
 
        if (ip) {
-               char *addr = inet_ntoa(*ip);
+               addr = inet_ntoa(*ip);
                setenv("ip", addr, 1);
-               bb_info_msg("%s %s %s", argv[1], intf, addr);
+               fmt -= 3;
        }
+       bb_info_msg(fmt, argv[1], intf, addr);
 
        status = wait4pid(spawn(argv));
        if (status < 0) {
-               bb_perror_msg("%s %s", argv[1], intf);
+               bb_perror_msg("%s %s %s" + 3, argv[1], intf);
                return -errno;
        }
        if (status != 0)
@@ -148,7 +164,7 @@ static int run(char *argv[3], const char *intf, struct in_addr *ip)
 /**
  * Return milliseconds of random delay, up to "secs" seconds.
  */
-static unsigned ALWAYS_INLINE ms_rdelay(unsigned secs)
+static unsigned ALWAYS_INLINE random_delay_ms(unsigned secs)
 {
        return rand() % (secs * 1000);
 }
@@ -160,10 +176,8 @@ int zcip_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
 int zcip_main(int argc, char **argv)
 {
        int state = PROBE;
-       /* Prevent unaligned traps for ARM (see srand() below) */
-       struct ether_addr eth_addr __attribute__(( aligned(sizeof(unsigned)) ));
+       struct ether_addr eth_addr;
        const char *why;
-       int fd;
        char *r_opt;
        unsigned opts;
 
@@ -171,10 +185,8 @@ int zcip_main(int argc, char **argv)
        struct {
                const struct in_addr null_ip;
                const struct ether_addr null_addr;
-               struct sockaddr saddr;
                struct in_addr ip;
                struct ifreq ifr;
-               char *intf;
                char *script_av[3];
                int timeout_ms; /* must be signed */
                unsigned conflicts;
@@ -185,10 +197,8 @@ int zcip_main(int argc, char **argv)
        } L;
 #define null_ip    (L.null_ip   )
 #define null_addr  (L.null_addr )
-#define saddr      (L.saddr     )
 #define ip         (L.ip        )
 #define ifr        (L.ifr       )
-#define intf       (L.intf      )
 #define script_av  (L.script_av )
 #define timeout_ms (L.timeout_ms)
 #define conflicts  (L.conflicts )
@@ -231,29 +241,37 @@ int zcip_main(int argc, char **argv)
 
        // initialize the interface (modprobe, ifup, etc)
        script_av[1] = (char*)"init";
-       if (run(script_av, intf, NULL))
+       if (run(script_av, NULL))
                return EXIT_FAILURE;
 
        // initialize saddr
+       // saddr is: { u16 sa_family; u8 sa_data[14]; }
        //memset(&saddr, 0, sizeof(saddr));
+       //TODO: are we leaving sa_family == 0 (AF_UNSPEC)?!
        safe_strncpy(saddr.sa_data, intf, sizeof(saddr.sa_data));
 
        // open an ARP socket
-       fd = xsocket(PF_PACKET, SOCK_PACKET, htons(ETH_P_ARP));
+       xmove_fd(xsocket(AF_PACKET, SOCK_PACKET, htons(ETH_P_ARP)), sock_fd);
        // bind to the interface's ARP socket
-       xbind(fd, &saddr, sizeof(saddr));
+       xbind(sock_fd, &saddr, sizeof(saddr));
 
        // get the interface's ethernet address
        //memset(&ifr, 0, sizeof(ifr));
        strncpy(ifr.ifr_name, intf, sizeof(ifr.ifr_name));
-       xioctl(fd, SIOCGIFHWADDR, &ifr);
+       xioctl(sock_fd, SIOCGIFHWADDR, &ifr);
        memcpy(&eth_addr, &ifr.ifr_hwaddr.sa_data, ETH_ALEN);
 
        // start with some stable ip address, either a function of
        // the hardware address or else the last address we used.
+       // we are taking low-order four bytes, as top-order ones
+       // aren't random enough.
        // NOTE: the sequence of addresses we try changes only
        // depending on when we detect conflicts.
-       srand(*(unsigned*)&eth_addr);
+       {
+               uint32_t t;
+               memcpy(&t, (char*)&eth_addr + 2, 4);
+               srand(t);
+       }
        if (ip.s_addr == 0)
                pick(&ip);
 
@@ -281,18 +299,17 @@ int zcip_main(int argc, char **argv)
                struct pollfd fds[1];
                unsigned deadline_us;
                struct arp_packet p;
+               int source_ip_conflict;
+               int target_ip_conflict;
 
-               int source_ip_conflict = 0;
-               int target_ip_conflict = 0;
-
-               fds[0].fd = fd;
+               fds[0].fd = sock_fd;
                fds[0].events = POLLIN;
                fds[0].revents = 0;
 
                // poll, being ready to adjust current timeout
                if (!timeout_ms) {
-                       timeout_ms = ms_rdelay(PROBE_WAIT);
-                       // FIXME setsockopt(fd, SO_ATTACH_FILTER, ...) to
+                       timeout_ms = random_delay_ms(PROBE_WAIT);
+                       // FIXME setsockopt(sock_fd, SO_ATTACH_FILTER, ...) to
                        // make the kernel filter out all packets except
                        // ones we'd care about.
                }
@@ -302,6 +319,8 @@ int zcip_main(int argc, char **argv)
                VDBG("...wait %d %s nprobes=%u, nclaims=%u\n",
                                timeout_ms, intf, nprobes, nclaims);
 
+       // FIXME: do we really receive ALL packets here??
+       // if yes, set up filtering to get ARPs only!!! (see arping)
                switch (safe_poll(fds, 1, timeout_ms)) {
 
                default:
@@ -319,11 +338,11 @@ int zcip_main(int argc, char **argv)
                                        nprobes++;
                                        VDBG("probe/%u %s@%s\n",
                                                        nprobes, intf, inet_ntoa(ip));
-                                       arp(fd, &saddr, ARPOP_REQUEST,
+                                       arp(ARPOP_REQUEST,
                                                        &eth_addr, null_ip,
                                                        &null_addr, ip);
                                        timeout_ms = PROBE_MIN * 1000;
-                                       timeout_ms += ms_rdelay(PROBE_MAX - PROBE_MIN);
+                                       timeout_ms += random_delay_ms(PROBE_MAX - PROBE_MIN);
                                }
                                else {
                                        // Switch to announce state.
@@ -331,7 +350,7 @@ int zcip_main(int argc, char **argv)
                                        nclaims = 0;
                                        VDBG("announce/%u %s@%s\n",
                                                        nclaims, intf, inet_ntoa(ip));
-                                       arp(fd, &saddr, ARPOP_REQUEST,
+                                       arp(ARPOP_REQUEST,
                                                        &eth_addr, ip,
                                                        &eth_addr, ip);
                                        timeout_ms = ANNOUNCE_INTERVAL * 1000;
@@ -344,7 +363,7 @@ int zcip_main(int argc, char **argv)
                                nclaims = 0;
                                VDBG("announce/%u %s@%s\n",
                                                nclaims, intf, inet_ntoa(ip));
-                               arp(fd, &saddr, ARPOP_REQUEST,
+                               arp(ARPOP_REQUEST,
                                                &eth_addr, ip,
                                                &eth_addr, ip);
                                timeout_ms = ANNOUNCE_INTERVAL * 1000;
@@ -356,7 +375,7 @@ int zcip_main(int argc, char **argv)
                                        nclaims++;
                                        VDBG("announce/%u %s@%s\n",
                                                        nclaims, intf, inet_ntoa(ip));
-                                       arp(fd, &saddr, ARPOP_REQUEST,
+                                       arp(ARPOP_REQUEST,
                                                        &eth_addr, ip,
                                                        &eth_addr, ip);
                                        timeout_ms = ANNOUNCE_INTERVAL * 1000;
@@ -367,7 +386,7 @@ int zcip_main(int argc, char **argv)
                                        // link is ok to use earlier
                                        // FIXME update filters
                                        script_av[1] = (char*)"config";
-                                       run(script_av, intf, &ip);
+                                       run(script_av, &ip);
                                        ready = 1;
                                        conflicts = 0;
                                        timeout_ms = -1; // Never timeout in the monitor state.
@@ -395,7 +414,7 @@ int zcip_main(int argc, char **argv)
                        } // switch (state)
                        break; // case 0 (timeout)
 
-               // packets arriving
+               // packets arriving, or link went down
                case 1:
                        // We need to adjust the timeout in case we didn't receive
                        // a conflicting packet.
@@ -408,8 +427,7 @@ int zcip_main(int argc, char **argv)
                                        timeout_ms = 0;
                                } else {
                                        VDBG("adjusting timeout\n");
-                                       timeout_ms = diff / 1000;
-                                       if (!timeout_ms) timeout_ms = 1;
+                                       timeout_ms = (diff / 1000) | 1; /* never 0 */
                                }
                        }
 
@@ -417,10 +435,10 @@ int zcip_main(int argc, char **argv)
                                if (fds[0].revents & POLLERR) {
                                        // FIXME: links routinely go down;
                                        // this shouldn't necessarily exit.
-                                       bb_error_msg("%s: poll error", intf);
+                                       bb_error_msg("iface %s is down", intf);
                                        if (ready) {
                                                script_av[1] = (char*)"deconfig";
-                                               run(script_av, intf, &ip);
+                                               run(script_av, &ip);
                                        }
                                        return EXIT_FAILURE;
                                }
@@ -428,21 +446,20 @@ int zcip_main(int argc, char **argv)
                        }
 
                        // read ARP packet
-                       if (recv(fd, &p, sizeof(p), 0) < 0) {
+                       if (safe_read(sock_fd, &p, sizeof(p)) < 0) {
                                why = "recv";
                                goto bad;
                        }
-                       if (p.hdr.ether_type != htons(ETHERTYPE_ARP))
+                       if (p.eth.ether_type != htons(ETHERTYPE_ARP))
                                continue;
-
 #ifdef DEBUG
                        {
-                               struct ether_addr * sha = (struct ether_addr *) p.arp.arp_sha;
-                               struct ether_addr * tha = (struct ether_addr *) p.arp.arp_tha;
-                               struct in_addr * spa = (struct in_addr *) p.arp.arp_spa;
-                               struct in_addr * tpa = (struct in_addr *) p.arp.arp_tpa;
+                               struct ether_addr *sha = (struct ether_addr *) p.arp.arp_sha;
+                               struct ether_addr *tha = (struct ether_addr *) p.arp.arp_tha;
+                               struct in_addr *spa = (struct in_addr *) p.arp.arp_spa;
+                               struct in_addr *tpa = (struct in_addr *) p.arp.arp_tpa;
                                VDBG("%s recv arp type=%d, op=%d,\n",
-                                       intf, ntohs(p.hdr.ether_type),
+                                       intf, ntohs(p.eth.ether_type),
                                        ntohs(p.arp.arp_op));
                                VDBG("\tsource=%s %s\n",
                                        ether_ntoa(sha),
@@ -453,16 +470,21 @@ int zcip_main(int argc, char **argv)
                        }
 #endif
                        if (p.arp.arp_op != htons(ARPOP_REQUEST)
-                                       && p.arp.arp_op != htons(ARPOP_REPLY))
+                        && p.arp.arp_op != htons(ARPOP_REPLY))
                                continue;
 
-                       if (memcmp(p.arp.arp_spa, &ip.s_addr, sizeof(struct in_addr)) == 0 &&
-                               memcmp(&eth_addr, &p.arp.arp_sha, ETH_ALEN) != 0) {
+                       source_ip_conflict = 0;
+                       target_ip_conflict = 0;
+
+                       if (memcmp(p.arp.arp_spa, &ip.s_addr, sizeof(struct in_addr)) == 0
+                        && memcmp(&p.arp.arp_sha, &eth_addr, ETH_ALEN) != 0
+                       ) {
                                source_ip_conflict = 1;
                        }
-                       if (memcmp(p.arp.arp_tpa, &ip.s_addr, sizeof(struct in_addr)) == 0 &&
-                               p.arp.arp_op == htons(ARPOP_REQUEST) &&
-                               memcmp(&eth_addr, &p.arp.arp_tha, ETH_ALEN) != 0) {
+                       if (p.arp.arp_op == htons(ARPOP_REQUEST)
+                        && memcmp(p.arp.arp_tpa, &ip.s_addr, sizeof(struct in_addr)) == 0
+                        && memcmp(&p.arp.arp_tha, &eth_addr, ETH_ALEN) != 0
+                       ) {
                                target_ip_conflict = 1;
                        }
 
@@ -494,10 +516,9 @@ int zcip_main(int argc, char **argv)
                                        VDBG("monitor conflict -- defending\n");
                                        state = DEFEND;
                                        timeout_ms = DEFEND_INTERVAL * 1000;
-                                       arp(fd, &saddr,
-                                                       ARPOP_REQUEST,
-                                                       &eth_addr, ip,
-                                                       &eth_addr, ip);
+                                       arp(ARPOP_REQUEST,
+                                               &eth_addr, ip,
+                                               &eth_addr, ip);
                                }
                                break;
                        case DEFEND:
@@ -507,7 +528,7 @@ int zcip_main(int argc, char **argv)
                                        VDBG("defend conflict -- starting over\n");
                                        ready = 0;
                                        script_av[1] = (char*)"deconfig";
-                                       run(script_av, intf, &ip);
+                                       run(script_av, &ip);
 
                                        // restart the whole protocol
                                        pick(&ip);
@@ -530,6 +551,6 @@ int zcip_main(int argc, char **argv)
                } // switch poll
        } // while (1)
  bad:
-       bb_perror_msg("%s, %s", intf, why);
+       bb_perror_msg("%s: %s", intf, why);
        return EXIT_FAILURE;
 }