[PATCH] ntpd: improve postponed hostname resolution
walter harms
wharms at bfs.de
Thu Jan 19 12:55:17 UTC 2017
Am 06.01.2017 12:13, schrieb Natanael Copa:
> 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'
> ...
>
> This patch is based on Kaarle Ritvanens work.
> http://lists.busybox.net/pipermail/busybox/2016-May/084197.html
>
> function old new delta
> static.set_next - 23 +23
> step_time 426 422 -4
> recv_and_process_peer_pkt 2342 2338 -4
> ntpd_main 1178 1171 -7
> add_peers 229 217 -12
> resolve_peer_hostname 102 88 -14
> .rodata 125081 125065 -16
> ------------------------------------------------------------------------------
> (add/remove: 1/0 grow/shrink: 0/6 up/down: 23/-57) Total: -34
> bytes
> text data bss dec hex filename
> 802129 15459 2192 819780 c8244 busybox_old
> 802095 15459 2192 819746 c8222 busybox_unstripped
>
> Signed-off-by: Kaarle Ritvanen <kaarle.ritvanen at datakunkku.fi>
> Signed-off-by: Natanael Copa <ncopa at alpinelinux.org>
> ---
> networking/ntpd.c | 71 ++++++++++++++++++++++++++-----------------------------
> 1 file changed, 33 insertions(+), 38 deletions(-)
>
> diff --git a/networking/ntpd.c b/networking/ntpd.c
> index b7fa5dce9..97e3919fb 100644
> --- 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 4 /* hostname lookup failed. Wait N secs for next try */
>
> /* Step threshold (sec). std ntpd uses 0.128.
> */
> @@ -293,6 +294,7 @@ typedef struct {
>
> typedef struct {
> len_and_sockaddr *p_lsa;
> + char *p_hostname;
> char *p_dotted;
> int p_fd;
> int datapoint_idx;
> @@ -318,7 +320,6 @@ typedef struct {
> datapoint_t filter_datapoint[NUM_DATAPOINTS];
> /* last sent packet: */
> msg_t p_xmt_msg;
> - char p_hostname[1];
> } peer_t;
>
>
> @@ -791,27 +792,17 @@ reset_peer_stats(peer_t *p, double offset)
> }
>
> static void
> -resolve_peer_hostname(peer_t *p, int loop_on_fail)
> -{
> - 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;
> +resolve_peer_hostname(peer_t *p) {
> + len_and_sockaddr *lsa = host2sockaddr(p->p_hostname, 123);
> + if (lsa) {
> + if (p->p_lsa) {
> + free(p->p_lsa);
> + free(p->p_dotted);
> + }
free will accept NULL so you can drop the if, or is there an other problem ?
> + p->p_lsa = lsa;
> + p->p_dotted = xmalloc_sockaddr2dotted_noport(&lsa->u.sa);
> }
> - free(p->p_lsa);
> - free(p->p_dotted);
> - p->p_lsa = lsa;
> - p->p_dotted = xmalloc_sockaddr2dotted_noport(&lsa->u.sa);
> + set_next(p, lsa ? 0 : HOSTNAME_INTERVAL);
is it possible to move the set_next into the if(lsa) block above
i feel that would improve readability.
re,
wh
> }
>
> static void
> @@ -820,28 +811,29 @@ add_peers(const char *s)
> llist_t *item;
> peer_t *p;
>
> - p = xzalloc(sizeof(*p) + strlen(s));
> - strcpy(p->p_hostname, s);
> - resolve_peer_hostname(p, /*loop_on_fail=*/ 1);
> + p = xzalloc(sizeof(*p));
> + p->p_hostname = xstrdup(s);
> + resolve_peer_hostname(p);
>
> /* 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 (p->p_lsa)
> + for (item = G.ntp_peers; item != NULL; item = item->link) {
> + peer_t *pp = (peer_t *) item->data;
> + if (pp->p_lsa && strcmp(p->p_dotted, pp->p_dotted) == 0) {
> + bb_error_msg("duplicate peer %s (%s)", s, p->p_dotted);
> + free(p->p_hostname);
> + 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);
> @@ -2263,8 +2255,8 @@ 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.
> + /* add_peers() calls can retry DNS resolution.
> + * Daemonize before them, or else boot can stall.
> */
> if (!(opts & OPT_n)) {
> bb_daemonize_or_rexec(DAEMON_DEVNULL_STDIO, argv);
> @@ -2378,7 +2370,10 @@ int ntpd_main(int argc UNUSED_PARAM, char **argv)
> for (item = G.ntp_peers; item != NULL; item = item->link) {
> peer_t *p = (peer_t *) item->data;
>
> - if (p->next_action_time <= G.cur_time) {
> + if (!p->p_lsa) {
> + resolve_peer_hostname(p);
> +
> + } else if (p->next_action_time <= G.cur_time) {
> if (p->p_fd == -1) {
> /* Time to send new req */
> if (--cnt == 0) {
> @@ -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);
> }
More information about the busybox
mailing list