Re: [chrony-dev] [PATCH] Port to MacOS X

[Miroslav Lichvar <mlichvar@xxxxxxxxxx>]

This looks pretty good. Some comments are inline.

I'm planning to make a 2.1 prerelease today or tomorrow (to have a new
release before the next leap second), but I think this could be
included in the final release if it's ready in time.

On Wed, Jun 10, 2015 at 03:43:23AM +1200, Bryan Christianson wrote:
>  sys_macos.c | 394 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  sys_macos.h |  36 ++++++

To avoid confusion with the older Mac OS system, would it make sense
to rename the files to sys_macosx.* or maybe sys_osx.*? I'm not sure
how developers usually call it.

Also, please mention in the commit message that it's based on the
NetBSD driver, so it's clear where the old copyrights came from.

>  clean :
> -	-rm -f *.o *.s chronyc chronyd core *~ chrony.info chrony.html chrony.txt
> +	-rm -f *.o *.s chronyc chronyd core *~ chrony.info chrony.html chrony.txt config.*
>  	-rm -rf .deps
> +	-rm -rf docheck.dSYM

What creates the config.* and docheck.dSYM files?

> @@ -112,6 +113,8 @@ install: chronyd chronyc chrony.txt
>  	chmod 644 $(DESTDIR)$(DOCDIR)/COPYING
>  	cp README $(DESTDIR)$(DOCDIR)/README
>  	chmod 644 $(DESTDIR)$(DOCDIR)/README
> +	cp chrony.1 $(DESTDIR)$(MANDIR)/man1
> +	chmod 644 $(DESTDIR)$(MANDIR)/man1/chrony.1

This looks like a parial revert of a commit added after 2.0, it
probably shouldn't be in this patch :).

> +    Darwin-* )
> +        EXTRA_OBJECTS="sys_macos.o"
> +        MYLDFLAGS="-lresolv" $MYLDFLAGS

The system specific libraries should be in EXTRA_LIBS, MYLDFLAGS is
for user specified flags.

Also, please consider using an add_def call here similarly to the
other systems (other than NetBSD) to have a define which could be used
in the C files instead of __APPLE__.

> --- /dev/null
> +++ b/sys_macos.c
> @@ -0,0 +1,394 @@
> +  =======================================================================
> +
> +  Driver file for the MacOS operating system.
> +  */

Mac OS X to avoid any confusion?

> +static inline void
> +ts_add_double(struct timespec *a, const struct timespec *b, double c)
> +{
> +	time_t secs;
> +	time_t nsecs;
> +	

The indentation is off here and there is also trailing whitespace.

> +	secs = (time_t)c;
> +	nsecs = (c - (time_t)c) * 1E9;
> +	
> +	secs += b->tv_sec;
> +	nsecs += b->tv_nsec;
> +	
> +	if(nsecs >= 1E9) {
> +		nsecs -= 1E9;
> +		secs++;
> +	}

Shouldn't 1E9 be 1000000000 here to avoid unnecessary conversions
to/from double?

> +/* ================================================== */
> +
> +#include <mach/clock.h>
> +#include <mach/mach.h>

There should go to the beginning of the file.

> +static inline void
> +getwallclock(struct timespec *ts)
> +{
> +#if 0
> +	struct timeval tv;
> +	
> +	if (gettimeofday(&tv, NULL) < 0) {
> +		LOG_FATAL(LOGF_SysMacOS, "gettimeofday() failed");
> +	}
> +	
> +	ts->tv_sec = tv.tv_sec;
> +	ts->tv_nsec = tv.tv_usec * 1000;
> +#else
> +	clock_serv_t cclock;
> +	mach_timespec_t mts;
> +
> +	host_get_clock_service(mach_host_self(), CALENDAR_CLOCK, &cclock);
> +	clock_get_time(cclock, &mts);
> +	mach_port_deallocate(mach_task_self(), cclock);
> +	
> +	ts->tv_sec = mts.tv_sec;
> +	ts->tv_nsec = mts.tv_nsec;
> +#endif

Hm, is clock_get_time() used here to get timestamps with nanosecond
resolution? How fast are the host_get_clock_service() and
mach_port_deallocate() calls?

If the T0 and T1 timestamps are used only to calculate the accrued
offset with frequency offset in few tens of ppm, I'm not sure
nanosecond resolution would improve anything here. I'd prefer to keep
the code simple and avoid any timeval<->timespec conversions until the
chrony code is converted to use timespec everywhere.

> +  /* At this point, we need to round the required adjustment the
> +     same way the kernel does. */
> +
> +  delta = adjust_required * 1E6;
> +  if (delta > kern_bigadj || delta < -kern_bigadj)
> +    tickdelta = 10 * kern_tickadj;
> +  else
> +    tickdelta = kern_tickadj;
> +  if (delta % tickdelta)
> +	delta = delta / tickdelta * tickdelta;

Does the rounding need to be the same as in the NetBSD driver? Do you
see any clock instability when it's removed?

> +void
> +SYS_MacOS_Initialise(void)
> +{
> +	size_t len;
> +	struct clockinfo clockinfo;
> +	int mib[2];
> +
> +	mib[0] = CTL_KERN;
> +	mib[1] = KERN_CLOCKRATE;
> +
> +	len = sizeof(clockinfo);
> +	int result = sysctl(mib, 2, &clockinfo, &len, NULL, 0);

The indentation is off here and the result declaration should be at
the beginning of the function.

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