Re: ntpd offset loop refactoring
On Tue, Apr 20, 2021 at 07:59:51PM +0200, Alexander Bluhm wrote:
> I got a positive test report from Paul de Weerd.
>
> Anyone to ok it?
OK claudio@
> On Thu, Mar 18, 2021 at 01:06:49PM +0100, Alexander Bluhm wrote:
> > Hi,
> >
> > While hunting a bug in ntpd offset handling, I found some things
> > that could be improved.
> >
> > Call the index of the offset loops "shift" consistently.
> > Merge the two offset loops in client_update() into one.
> > Assign the best value instead of memcpy.
> > Use the same mechanism everywhere to avoid an invalid best value.
> >
> > ok?
> >
> > bluhm
> >
> > Index: client.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ntpd/client.c,v
> > retrieving revision 1.115
> > diff -u -p -r1.115 client.c
> > --- client.c18 Mar 2021 11:06:41 - 1.115
> > +++ client.c18 Mar 2021 11:20:49 -
> > @@ -473,7 +473,7 @@ client_dispatch(struct ntp_peer *p, u_in
> > int
> > client_update(struct ntp_peer *p)
> > {
> > - int i, best = 0, good = 0;
> > + int shift, best = -1, good = 0;
> >
> > /*
> > * clock filter
> > @@ -482,27 +482,22 @@ client_update(struct ntp_peer *p)
> > * invalidate it and all older ones
> > */
> >
> > - for (i = 0; good == 0 && i < OFFSET_ARRAY_SIZE; i++)
> > - if (p->reply[i].good) {
> > + for (shift = 0; shift < OFFSET_ARRAY_SIZE; shift++)
> > + if (p->reply[shift].good) {
> > good++;
> > - best = i;
> > + if (best == -1 ||
> > + p->reply[shift].delay < p->reply[best].delay)
> > + best = shift;
> > }
> >
> > - for (; i < OFFSET_ARRAY_SIZE; i++)
> > - if (p->reply[i].good) {
> > - good++;
> > - if (p->reply[i].delay < p->reply[best].delay)
> > - best = i;
> > - }
> > -
> > - if (good < 8)
> > + if (best == -1 || good < 8)
> > return (-1);
> >
> > - memcpy(>update, >reply[best], sizeof(p->update));
> > + p->update = p->reply[best];
> > if (priv_adjtime() == 0) {
> > - for (i = 0; i < OFFSET_ARRAY_SIZE; i++)
> > - if (p->reply[i].rcvd <= p->reply[best].rcvd)
> > - p->reply[i].good = 0;
> > + for (shift = 0; shift < OFFSET_ARRAY_SIZE; shift++)
> > + if (p->reply[shift].rcvd <= p->reply[best].rcvd)
> > + p->reply[shift].good = 0;
> > }
> > return (0);
> > }
> > Index: control.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ntpd/control.c,v
> > retrieving revision 1.18
> > diff -u -p -r1.18 control.c
> > --- control.c 12 Feb 2020 19:14:56 - 1.18
> > +++ control.c 18 Mar 2021 11:20:53 -
> > @@ -341,7 +341,7 @@ build_show_peer(struct ctl_show_peer *cp
> > const char *a = "not resolved";
> > const char *pool = "", *addr_head_name = "";
> > const char *auth = "";
> > - u_int8_t shift, best, validdelaycnt, jittercnt;
> > + int shift, best = -1, validdelaycnt = 0, jittercnt = 0;
> > time_t now;
> >
> > now = getmonotime();
> > @@ -360,14 +360,14 @@ build_show_peer(struct ctl_show_peer *cp
> > snprintf(cp->peer_desc, sizeof(cp->peer_desc),
> > "%s %s%s%s", a, pool, addr_head_name, auth);
> >
> > - validdelaycnt = best = 0;
> > cp->offset = cp->delay = 0.0;
> > for (shift = 0; shift < OFFSET_ARRAY_SIZE; shift++) {
> > if (p->reply[shift].delay > 0.0) {
> > cp->offset += p->reply[shift].offset;
> > cp->delay += p->reply[shift].delay;
> >
> > - if (p->reply[shift].delay < p->reply[best].delay)
> > + if (best == -1 ||
> > + p->reply[shift].delay < p->reply[best].delay)
> > best = shift;
> >
> > validdelaycnt++;
> > @@ -379,18 +379,19 @@ build_show_peer(struct ctl_show_peer *cp
> > cp->delay /= validdelaycnt;
> > }
> >
> > - jittercnt = 0;
> > cp->jitter = 0.0;
> > - for (shift = 0; shift < OFFSET_ARRAY_SIZE; shift++) {
> > - if (p->reply[shift].delay > 0.0 && shift != best) {
> > - cp->jitter += square(p->reply[shift].delay -
> > - p->reply[best].delay);
> > - jittercnt++;
> > + if (best != -1) {
> > + for (shift = 0; shift < OFFSET_ARRAY_SIZE; shift++) {
> > + if (p->reply[shift].delay > 0.0 && shift != best) {
> > + cp->jitter += square(p->reply[shift].delay -
> > + p->reply[best].delay);
> > + jittercnt++;
> > + }
> > }
> > +
Re: ntpd offset loop refactoring
I got a positive test report from Paul de Weerd.
Anyone to ok it?
On Thu, Mar 18, 2021 at 01:06:49PM +0100, Alexander Bluhm wrote:
> Hi,
>
> While hunting a bug in ntpd offset handling, I found some things
> that could be improved.
>
> Call the index of the offset loops "shift" consistently.
> Merge the two offset loops in client_update() into one.
> Assign the best value instead of memcpy.
> Use the same mechanism everywhere to avoid an invalid best value.
>
> ok?
>
> bluhm
>
> Index: client.c
> ===
> RCS file: /cvs/src/usr.sbin/ntpd/client.c,v
> retrieving revision 1.115
> diff -u -p -r1.115 client.c
> --- client.c 18 Mar 2021 11:06:41 - 1.115
> +++ client.c 18 Mar 2021 11:20:49 -
> @@ -473,7 +473,7 @@ client_dispatch(struct ntp_peer *p, u_in
> int
> client_update(struct ntp_peer *p)
> {
> - int i, best = 0, good = 0;
> + int shift, best = -1, good = 0;
>
> /*
>* clock filter
> @@ -482,27 +482,22 @@ client_update(struct ntp_peer *p)
>* invalidate it and all older ones
>*/
>
> - for (i = 0; good == 0 && i < OFFSET_ARRAY_SIZE; i++)
> - if (p->reply[i].good) {
> + for (shift = 0; shift < OFFSET_ARRAY_SIZE; shift++)
> + if (p->reply[shift].good) {
> good++;
> - best = i;
> + if (best == -1 ||
> + p->reply[shift].delay < p->reply[best].delay)
> + best = shift;
> }
>
> - for (; i < OFFSET_ARRAY_SIZE; i++)
> - if (p->reply[i].good) {
> - good++;
> - if (p->reply[i].delay < p->reply[best].delay)
> - best = i;
> - }
> -
> - if (good < 8)
> + if (best == -1 || good < 8)
> return (-1);
>
> - memcpy(>update, >reply[best], sizeof(p->update));
> + p->update = p->reply[best];
> if (priv_adjtime() == 0) {
> - for (i = 0; i < OFFSET_ARRAY_SIZE; i++)
> - if (p->reply[i].rcvd <= p->reply[best].rcvd)
> - p->reply[i].good = 0;
> + for (shift = 0; shift < OFFSET_ARRAY_SIZE; shift++)
> + if (p->reply[shift].rcvd <= p->reply[best].rcvd)
> + p->reply[shift].good = 0;
> }
> return (0);
> }
> Index: control.c
> ===
> RCS file: /cvs/src/usr.sbin/ntpd/control.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 control.c
> --- control.c 12 Feb 2020 19:14:56 - 1.18
> +++ control.c 18 Mar 2021 11:20:53 -
> @@ -341,7 +341,7 @@ build_show_peer(struct ctl_show_peer *cp
> const char *a = "not resolved";
> const char *pool = "", *addr_head_name = "";
> const char *auth = "";
> - u_int8_t shift, best, validdelaycnt, jittercnt;
> + int shift, best = -1, validdelaycnt = 0, jittercnt = 0;
> time_t now;
>
> now = getmonotime();
> @@ -360,14 +360,14 @@ build_show_peer(struct ctl_show_peer *cp
> snprintf(cp->peer_desc, sizeof(cp->peer_desc),
> "%s %s%s%s", a, pool, addr_head_name, auth);
>
> - validdelaycnt = best = 0;
> cp->offset = cp->delay = 0.0;
> for (shift = 0; shift < OFFSET_ARRAY_SIZE; shift++) {
> if (p->reply[shift].delay > 0.0) {
> cp->offset += p->reply[shift].offset;
> cp->delay += p->reply[shift].delay;
>
> - if (p->reply[shift].delay < p->reply[best].delay)
> + if (best == -1 ||
> + p->reply[shift].delay < p->reply[best].delay)
> best = shift;
>
> validdelaycnt++;
> @@ -379,18 +379,19 @@ build_show_peer(struct ctl_show_peer *cp
> cp->delay /= validdelaycnt;
> }
>
> - jittercnt = 0;
> cp->jitter = 0.0;
> - for (shift = 0; shift < OFFSET_ARRAY_SIZE; shift++) {
> - if (p->reply[shift].delay > 0.0 && shift != best) {
> - cp->jitter += square(p->reply[shift].delay -
> - p->reply[best].delay);
> - jittercnt++;
> + if (best != -1) {
> + for (shift = 0; shift < OFFSET_ARRAY_SIZE; shift++) {
> + if (p->reply[shift].delay > 0.0 && shift != best) {
> + cp->jitter += square(p->reply[shift].delay -
> + p->reply[best].delay);
> + jittercnt++;
> + }
> }
> + if (jittercnt > 1)
> + cp->jitter /= jittercnt;
> + cp->jitter = sqrt(cp->jitter);
> }
> - if (jittercnt > 1)
> - cp->jitter /= jittercnt;
> -
ntpd offset loop refactoring
Hi,
While hunting a bug in ntpd offset handling, I found some things
that could be improved.
Call the index of the offset loops "shift" consistently.
Merge the two offset loops in client_update() into one.
Assign the best value instead of memcpy.
Use the same mechanism everywhere to avoid an invalid best value.
ok?
bluhm
Index: client.c
===
RCS file: /cvs/src/usr.sbin/ntpd/client.c,v
retrieving revision 1.115
diff -u -p -r1.115 client.c
--- client.c18 Mar 2021 11:06:41 - 1.115
+++ client.c18 Mar 2021 11:20:49 -
@@ -473,7 +473,7 @@ client_dispatch(struct ntp_peer *p, u_in
int
client_update(struct ntp_peer *p)
{
- int i, best = 0, good = 0;
+ int shift, best = -1, good = 0;
/*
* clock filter
@@ -482,27 +482,22 @@ client_update(struct ntp_peer *p)
* invalidate it and all older ones
*/
- for (i = 0; good == 0 && i < OFFSET_ARRAY_SIZE; i++)
- if (p->reply[i].good) {
+ for (shift = 0; shift < OFFSET_ARRAY_SIZE; shift++)
+ if (p->reply[shift].good) {
good++;
- best = i;
+ if (best == -1 ||
+ p->reply[shift].delay < p->reply[best].delay)
+ best = shift;
}
- for (; i < OFFSET_ARRAY_SIZE; i++)
- if (p->reply[i].good) {
- good++;
- if (p->reply[i].delay < p->reply[best].delay)
- best = i;
- }
-
- if (good < 8)
+ if (best == -1 || good < 8)
return (-1);
- memcpy(>update, >reply[best], sizeof(p->update));
+ p->update = p->reply[best];
if (priv_adjtime() == 0) {
- for (i = 0; i < OFFSET_ARRAY_SIZE; i++)
- if (p->reply[i].rcvd <= p->reply[best].rcvd)
- p->reply[i].good = 0;
+ for (shift = 0; shift < OFFSET_ARRAY_SIZE; shift++)
+ if (p->reply[shift].rcvd <= p->reply[best].rcvd)
+ p->reply[shift].good = 0;
}
return (0);
}
Index: control.c
===
RCS file: /cvs/src/usr.sbin/ntpd/control.c,v
retrieving revision 1.18
diff -u -p -r1.18 control.c
--- control.c 12 Feb 2020 19:14:56 - 1.18
+++ control.c 18 Mar 2021 11:20:53 -
@@ -341,7 +341,7 @@ build_show_peer(struct ctl_show_peer *cp
const char *a = "not resolved";
const char *pool = "", *addr_head_name = "";
const char *auth = "";
- u_int8_t shift, best, validdelaycnt, jittercnt;
+ int shift, best = -1, validdelaycnt = 0, jittercnt = 0;
time_t now;
now = getmonotime();
@@ -360,14 +360,14 @@ build_show_peer(struct ctl_show_peer *cp
snprintf(cp->peer_desc, sizeof(cp->peer_desc),
"%s %s%s%s", a, pool, addr_head_name, auth);
- validdelaycnt = best = 0;
cp->offset = cp->delay = 0.0;
for (shift = 0; shift < OFFSET_ARRAY_SIZE; shift++) {
if (p->reply[shift].delay > 0.0) {
cp->offset += p->reply[shift].offset;
cp->delay += p->reply[shift].delay;
- if (p->reply[shift].delay < p->reply[best].delay)
+ if (best == -1 ||
+ p->reply[shift].delay < p->reply[best].delay)
best = shift;
validdelaycnt++;
@@ -379,18 +379,19 @@ build_show_peer(struct ctl_show_peer *cp
cp->delay /= validdelaycnt;
}
- jittercnt = 0;
cp->jitter = 0.0;
- for (shift = 0; shift < OFFSET_ARRAY_SIZE; shift++) {
- if (p->reply[shift].delay > 0.0 && shift != best) {
- cp->jitter += square(p->reply[shift].delay -
- p->reply[best].delay);
- jittercnt++;
+ if (best != -1) {
+ for (shift = 0; shift < OFFSET_ARRAY_SIZE; shift++) {
+ if (p->reply[shift].delay > 0.0 && shift != best) {
+ cp->jitter += square(p->reply[shift].delay -
+ p->reply[best].delay);
+ jittercnt++;
+ }
}
+ if (jittercnt > 1)
+ cp->jitter /= jittercnt;
+ cp->jitter = sqrt(cp->jitter);
}
- if (jittercnt > 1)
- cp->jitter /= jittercnt;
- cp->jitter = sqrt(cp->jitter);
if (p->shift == 0)
shift = OFFSET_ARRAY_SIZE - 1;
