On Tue, Mar 13, 2018 at 10:45:56AM +0100, Chr?stian Ehrhardt wrote: > doc/chronyd.adoc | 7 +++++++ > examples/chronyd.service | 1 - > main.c | 10 +++++++--- > sys.c | 26 +++++++++++++++++++++----- > sys.h | 2 +- > sys_linux.c | 28 +++++++++++++++++++--------- > sys_linux.h | 4 +++- > sys_macosx.c | 8 ++++---- > sys_macosx.h | 2 +- > sys_netbsd.c | 8 ++++---- > sys_netbsd.h | 2 +- > sys_solaris.c | 6 +++--- > sys_solaris.h | 2 +- > sys_timex.c | 39 +++++++++++++++++++++++++++------------ > sys_timex.h | 16 ++++++++-------- > 15 files changed, 107 insertions(+), 54 deletions(-)
I think this should be split into three patches: - improve the error message when the first Linux adjtimex() fails to say it's missing the capability - rework the sys_* code to return a value from the initialization - add the fallback value and -X to main and documentation At this time, I'd be interested in including only in the first one. We can reconsider the other two later if you are still interested. > diff --git a/examples/chronyd.service b/examples/chronyd.service > index 4ffe3b1..b86bef6 100644 > --- a/examples/chronyd.service > +++ b/examples/chronyd.service > @@ -3,7 +3,6 @@ Description=NTP client/server > Documentation=man:chronyd(8) man:chrony.conf(5) > After=ntpdate.service sntp.service ntpd.service > Conflicts=ntpd.service systemd-timesyncd.service > -ConditionCapability=CAP_SYS_TIME > > [Service] > Type=forking The example unit file shouldn't change. > diff --git a/main.c b/main.c > index a2202e9..289a01d 100644 > --- a/main.c > +++ b/main.c > @@ -406,7 +406,8 @@ int main > int opt, debug = 0, nofork = 0, address_family = IPADDR_UNSPEC; > int do_init_rtc = 0, restarted = 0, client_only = 0, timeout = 0; > int scfilter_level = 0, lock_memory = 0, sched_priority = 0; > - int clock_control = 1, system_log = 1; > + int clock_control = 1, clock_fallback = 0; I'd prefer if -X set clock_control to -1 (for automatic selection) instead of adding a new parameter. > --- a/sys.c > +++ b/sys.c > @@ -50,24 +50,40 @@ static int null_driver; > /* ================================================== */ > > void > -SYS_Initialise(int clock_control) > +SYS_Initialise(int clock_control, int clock_fallback) > { > + int initalized = 0; initialised (British spelling) > + > null_driver = !clock_control; > if (null_driver) { > SYS_Null_Initialise(); > return; > } > #if defined(LINUX) > - SYS_Linux_Initialise(); > + initalized = SYS_Linux_Initialise(); > #elif defined(SOLARIS) > - SYS_Solaris_Initialise(); > + initalized = SYS_Solaris_Initialise(); > #elif defined(NETBSD) || defined(FREEBSD) > - SYS_NetBSD_Initialise(); > + initalized = SYS_NetBSD_Initialise(); > #elif defined(MACOSX) > - SYS_MacOSX_Initialise(); > + initalized = SYS_MacOSX_Initialise(); > #else > #error Unknown system > #endif > + > + if (!initalized) { > + LOG(LOGS_WARN, "Failed to initialize control of local system clock"); This could be a (fatal if clock_control > 0) error. I'd suggest to use a bit shorter "Could not initialise system clock driver". > +#if defined(LINUX) && defined (FEAT_PRIVDROP) > + SYS_Linux_ReportTimeAdjustBlockers(); > +#endif Could this be included in SYS_Linux_Initialise()? > + if (!clock_fallback) { > + LOG_FATAL("No Fallback (-X) allowed, init failed"); > + } else { > + LOG(LOGS_WARN, "Falling back by disabling control of the system > clock"); I'd remove these messages. > +void SYS_Linux_ReportTimeAdjustBlockers(void) > +{ > + if (CAP_IS_SUPPORTED(CAP_SYS_TIME) && cap_get_bound(CAP_SYS_TIME)) > + return; > + LOG(LOGS_WARN, "CAP_SYS_TIME not present"); > +} Could this be included (and wrapped in FEAT_PRIVDROP) in SYS_Linux_Initialise()? Thanks, -- Miroslav Lichvar -- To unsubscribe email chrony-dev-requ...@chrony.tuxfamily.org with "unsubscribe" in the subject. For help email chrony-dev-requ...@chrony.tuxfamily.org with "help" in the subject. Trouble? Email listmas...@chrony.tuxfamily.org.