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

[Bryan Christianson <bryan@xxxxxxxxxxxxx>]

Think I fixed all the things commented on.
I removed the nanosecond calls/conversions so the patch to the driver is now much simpler.

Attachment: 0001-MacOS-X-driver-ported-from-NetBSD.patch
Description: Binary data

> On 10/06/2015, at 8:21 pm, Miroslav Lichvar <mlichvar@xxxxxxxxxx> wrote:
> 
> 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.
>