[prev in list] [next in list] [prev in thread] [next in thread]
List: busybox
Subject: Re: [PATCH] ntpd: improve postponed hostname resolution
From: Denys Vlasenko <vda.linux () googlemail ! com>
Date: 2017-01-19 13:30:59
Message-ID: CAK1hOcPH41zeEE=fg2AzCc5EBnxL6LMhuQEP2cTCXcWjGZ632g () mail ! gmail ! com
[Download RAW message or body]
walter, check current git, it's a bit different there
On Thu, Jan 19, 2017 at 1:55 PM, walter harms <wharms@bfs.de> wrote:
>
>
> 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@datakunkku.fi>
>> Signed-off-by: Natanael Copa <ncopa@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);
>> }
> _______________________________________________
> busybox mailing list
> busybox@busybox.net
> http://lists.busybox.net/mailman/listinfo/busybox
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic