Hi Frank :)
On 31/01/2021 07:58, Frank Kardel wrote:
For example I fail to see how RTM_LOSING helps that because it won't
change
how ntpd would configure itself.
Well if I read the comment right I am inclined to differ here:
In in_pcs.c we find:
/*
* Check for alternatives when higher level complains
* about service problems. For now, invalidate cached
* routing information. If the route was created dynamically
* (by a redirect), time to try a default gateway again.
*/
in_losing(struct inpcb *inp)
and the call is in tcp_time.c:
/*
* If losing, let the lower level know and try for
* a better route. Also, if we backed off this far,
* our srtt estimate is probably bogus. Clobber it
* so we'll take the next rtt measurement as our srtt;
* move the current srtt into rttvar to keep the current
* retransmit times until then.
*/
As ntpd acts after a grace period the routing engine may have
corrected this situation and routing may indeed change.
ntpd's interactions with peers can take up to 1024s so it is good to
attempt in a best effort way to keep the internal
local address/socket state close to the current state.
It is likely though that there have been routing messages like
RTM_CHANGE/ADD/DELETE before that and RTM_LOSING is not providing
additional information at the point.
Right, RTM_LOSING is just informational.
If any routing does change then we get RTM_CHANGE/ADD/DELETE etc.
As NTP doesn't bring interfaces up or down, RFM_IFANNOUNCE is
useless as well.
If the interface does vanish, any addresses on it will be reported
via RTM_DELADDR.
RTM_IFINFO is also questionable as commentary in the code is that it
only cares about addresses.
Well I read
ntp_io.c
/*
* we are keen on new and deleted addresses and
* if an interface goes up and down or routing
* changes
*/
not as being interested in addresses only.
Also keep in mind that at this point routing messages are processed
in a loop and the action here
timer_interfacetimeout(current_time + UPDATE_GRACE);
just sets the variable for the next interface+local address update
run. This is very cheap. The grace period
will batch multiple routing message together. An explicit routing
message flush is from my point of view
code clutter here. as the socket is effectively drained in the loop
at the cost of examining the msg_type and setting
a variable. Not much gained here.
OK, we'll keep RTM_IFINFO but drop RTM_IFANNOUNCE.
The point is trying to eliminate the overflow message entirely.
I mean, if you want to argue against any of that then I would
suggest why even bother filtering or looking at overflow at all?
Shrink the code - any activity on the routing socket, drain it
ignoring all error, start the interface update timer.
That would be an option but we should react only on known events.
There may be one or two events that could be removed from
the list after examination as other messages can cover for them. Keep
in mind the this is a portable code section and the
code tries to be on the fail safe, robust side for the goal of
address/routing tracking so adjusting it to a particular implementation
may break on other os implementations.
Well, Dragonfly (prior to my patches there) and by extension FreeBSD
(not checked to see if that changed) both emit RMT_DELADDR before
RTM_IFANNOUNCE (ie wrong order) so when they do overflow you never see
RTM_IFANNOUNCE to say the interface vanished. Hence there is zero
point is listening for it for ntp.
As for the message: IMHO it does not need to be logged at all
(DPRINTF/maybe LOGDEBUG at most) because the overflow should and
does just trigger ntpd to reevaluate the interface/routing
configuration.
This information is not important at all for normal operation as
the effects are correctly mitigated.
I changed it to LOG_DEBUG as well as removing RTM_LOSING and
RTM_IFANNOUNCE as discussed above.
Great.
BTW: does the current code revert to (fail safe) periodic interface
scanning if the routing socket is being disabled (happens when an
unexpected error code is returned from read(2))?
No.
The socket is non blocking so the only error to ignore here would be
EINTR.
Any other errors are due to bad programming IMO.
Could be bad programming, but I prefer the ntpd being forgiving
against hiccups by reverting to periodic scanning when we
disable to routing socket. That is a fail safe strategy and would
also warrant a log message as it is an unusual event.
EINTR is now ignored.
I'll find time to restore periodic scanning later.
Roy