Re: [chrony-dev] [PATCH v3] main: add -X to fall back if time is not adjustable

[Miroslav Lichvar <mlichvar@xxxxxxxxxx>]

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-request@xxxxxxxxxxxxxxxxxxxx with "unsubscribe" in the subject.
For help email chrony-dev-request@xxxxxxxxxxxxxxxxxxxx with "help" in the subject.
Trouble?  Email listmaster@xxxxxxxxxxxxxxxxxxxx.