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

[Christian Ehrhardt <christian.ehrhardt@xxxxxxxxxxxxx>]

On Wed, Mar 14, 2018 at 11:01 AM, Miroslav Lichvar <mlichvar@xxxxxxxxxx> wrote:

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

I agree and will do that - also for most of the inline feedback below. 

I'll reply to the one I'd challenge inline as well (on the .service file changes)

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.

 

Worst case we at least improve the messaging which is better than nothing.

That can be ok depending on what "later" means, what timeline are we talking about for "later"?

In March is kind of ok, >=April would likely be too late for me.

> 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.

Well, without dropping ConditionCapability=CAP_SYS_TIME it will never try to use it.

No matter if one configures it for -x or the new -X - it will just not even try while it would work inside the limits of -x/-X in those cases.

 

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

--

Christian Ehrhardt

Software Engineer, Ubuntu Server

Canonical Ltd