ntpd: improve postponed hostname resolution

Run the namelookup from the main loop so a misspelled first ntp server
name does not block everything forever.

This fixes the following situation which would block forever:
  $ sudo ./busybox ntpd -dn -p foobar  -p pool.ntp.org
  ntpd: bad address 'foobar'
  ntpd: bad address 'foobar'
  ntpd: bad address 'foobar'
  ...

New behavior:
  ntpd: bad address 'foobar'
  ntpd: sending query to 137.190.2.4
  ntpd: reply from 137.190.2.4: offset:-1.009775 delay:0.175550 status:0x24 strat:1 refid:0x00535047 rootdelay:0.000000 reach:0x01
  ntpd: sending query to 137.190.2.4
  ntpd: reply from 137.190.2.4: offset:-1.009605 delay:0.175461 status:0x24 strat:1 refid:0x00535047 rootdelay:0.000000 reach:0x03
  ntpd: sending query to 137.190.2.4
  ntpd: reply from 137.190.2.4: offset:-1.005327 delay:0.167027 status:0x24 strat:1 refid:0x00535047 rootdelay:0.000000 reach:0x07
  ntpd: sending query to 137.190.2.4
  ntpd: bad address 'foobar'
  ntpd: reply from 137.190.2.4: offset:-1.046349 delay:0.248705 status:0x24 strat:1 refid:0x00535047 rootdelay:0.000000 reach:0x0f

This patch is based on Kaarle Ritvanens work.
http://lists.busybox.net/pipermail/busybox/2016-May/084197.html

function                                             old     new   delta
ntpd_main                                           1061    1079     +18
ntp_init                                             556     560      +4
resolve_peer_hostname                                 81      75      -6
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 2/1 up/down: 22/-6)              Total: 16 bytes

Signed-off-by: Kaarle Ritvanen <kaarle.ritvanen@datakunkku.fi>
Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>

diff --git a/networking/ntpd.c b/networking/ntpd.c
index b7fa5dce9544ad7582455fbac30aa0190121ff29..bfd5705fcd30a8ab2293eec6baa2c7a02ec6c119 100644 (file)
--- a/networking/ntpd.c
+++ b/networking/ntpd.c
@@ -155,6 +155,7 @@
 #define RETRY_INTERVAL    32    /* on send/recv error, retry in N secs (need to be power of 2) */
 #define NOREPLY_INTERVAL 512    /* sent, but got no reply: cap next query by this many seconds */
 #define RESPONSE_INTERVAL 16    /* wait for reply up to N secs */
+#define HOSTNAME_INTERVAL  5    /* hostname lookup failed. Wait N secs for next try */
 
 /* Step threshold (sec). std ntpd uses 0.128.
  */
@@ -790,28 +791,20 @@ reset_peer_stats(peer_t *p, double offset)
        VERB6 bb_error_msg("%s->lastpkt_recv_time=%f", p->p_dotted, p->lastpkt_recv_time);
 }
 
-static void
-resolve_peer_hostname(peer_t *p, int loop_on_fail)
+static len_and_sockaddr*
+resolve_peer_hostname(peer_t *p)
 {
-       len_and_sockaddr *lsa;
-
- again:
-       lsa = host2sockaddr(p->p_hostname, 123);
-       if (!lsa) {
-               /* error message already emitted by host2sockaddr() */
-               if (!loop_on_fail)
-                       return;
-//FIXME: do this to avoid infinite looping on typo in a hostname?
-//well... in which case, what is a good value for loop_on_fail?
-               //if (--loop_on_fail == 0)
-               //      xfunc_die();
-               sleep(5);
-               goto again;
+       len_and_sockaddr *lsa = host2sockaddr(p->p_hostname, 123);
+       if (lsa) {
+               free(p->p_lsa);
+               free(p->p_dotted);
+               p->p_lsa = lsa;
+               p->p_dotted = xmalloc_sockaddr2dotted_noport(&lsa->u.sa);
+       } else {
+               /* error message is emitted by host2sockaddr() */
+               set_next(p, HOSTNAME_INTERVAL);
        }
-       free(p->p_lsa);
-       free(p->p_dotted);
-       p->p_lsa = lsa;
-       p->p_dotted = xmalloc_sockaddr2dotted_noport(&lsa->u.sa);
+       return lsa;
 }
 
 static void
@@ -822,28 +815,28 @@ add_peers(const char *s)
 
        p = xzalloc(sizeof(*p) + strlen(s));
        strcpy(p->p_hostname, s);
-       resolve_peer_hostname(p, /*loop_on_fail=*/ 1);
+       p->p_fd = -1;
+       p->p_xmt_msg.m_status = MODE_CLIENT | (NTP_VERSION << 3);
+       p->next_action_time = G.cur_time; /* = set_next(p, 0); */
+       reset_peer_stats(p, STEP_THRESHOLD);
 
        /* Names like N.<country2chars>.pool.ntp.org are randomly resolved
         * to a pool of machines. Sometimes different N's resolve to the same IP.
         * It is not useful to have two peers with same IP. We skip duplicates.
         */
-       for (item = G.ntp_peers; item != NULL; item = item->link) {
-               peer_t *pp = (peer_t *) item->data;
-               if (strcmp(p->p_dotted, pp->p_dotted) == 0) {
-                       bb_error_msg("duplicate peer %s (%s)", s, p->p_dotted);
-                       free(p->p_lsa);
-                       free(p->p_dotted);
-                       free(p);
-                       return;
+       if (resolve_peer_hostname(p)) {
+               for (item = G.ntp_peers; item != NULL; item = item->link) {
+                       peer_t *pp = (peer_t *) item->data;
+                       if (pp->p_dotted && strcmp(p->p_dotted, pp->p_dotted) == 0) {
+                               bb_error_msg("duplicate peer %s (%s)", s, p->p_dotted);
+                               free(p->p_lsa);
+                               free(p->p_dotted);
+                               free(p);
+                               return;
+                       }
                }
        }
 
-       p->p_fd = -1;
-       p->p_xmt_msg.m_status = MODE_CLIENT | (NTP_VERSION << 3);
-       p->next_action_time = G.cur_time; /* = set_next(p, 0); */
-       reset_peer_stats(p, STEP_THRESHOLD);
-
        llist_add_to(&G.ntp_peers, p);
        G.peer_cnt++;
 }
@@ -871,6 +864,11 @@ do_sendto(int fd,
 static void
 send_query_to_peer(peer_t *p)
 {
+       if (!p->p_lsa) {
+               if (!resolve_peer_hostname(p))
+                       return;
+       }
+
        /* Why do we need to bind()?
         * See what happens when we don't bind:
         *
@@ -2238,7 +2236,7 @@ static NOINLINE void ntp_init(char **argv)
                        IF_FEATURE_NTPD_SERVER("I:") /* compat */
                        "d" /* compat */
                        "46aAbgL", /* compat, ignored */
-                       &peers,&G.script_name,
+                       &peers, &G.script_name,
 #if ENABLE_FEATURE_NTPD_SERVER
                        &G.if_name,
 #endif
@@ -2263,9 +2261,6 @@ static NOINLINE void ntp_init(char **argv)
        if (opts & OPT_N)
                setpriority(PRIO_PROCESS, 0, -15);
 
-       /* add_peers() calls can retry DNS resolution (possibly forever).
-        * Daemonize before them, or else boot can stall forever.
-        */
        if (!(opts & OPT_n)) {
                bb_daemonize_or_rexec(DAEMON_DEVNULL_STDIO, argv);
                logmode = LOGMODE_NONE;
@@ -2400,7 +2395,7 @@ int ntpd_main(int argc UNUSED_PARAM, char **argv)
 
                                        /* What if don't see it because it changed its IP? */
                                        if (p->reachable_bits == 0)
-                                               resolve_peer_hostname(p, /*loop_on_fail=*/ 0);
+                                               resolve_peer_hostname(p);
 
                                        set_next(p, timeout);
                                }