[Openais] [corosync] [patch] - Use clock_gettime for timers

[Steven Dake]

In general the patch looks very good.  The issues are as follows:

1)

corosync_timer_add_absolute expects the expire time to be the
nanoseconds since the epoch, or more specifically, 

from the time(7) man page:
   The Epoch
       Unix  systems  represent  time  in  seconds  since  the Epoch,
which is
       defined as 0:00:00 UTC on the morning of 1 January 1970.

In this case, the expire_time may not be sufficient.  We want absolute
timers to match the wall clock time rather then be based upon the offset
from the current time.  If that is not the case with your patch, that
needs to be fixed.  absolute timers are used by services to expire
timers when a specific wall clock time has passed.  So in this case, if
ntp moves the time forward, we want that timer to expire, if its
recorded wall clock time is past the current time, the timer should be
expired.

2) souce should be spelled source in configure.ac

also the logic to detect monotonic clocks in configure.ac seems complex.
I'd like Jim to have a look at the configure.ac to determine if there is
a better way.

3) get_monotonic_timeofday used in pload.c and votequorum.c and
totemsrp.c should be in tlist.h and should be named differently.  I'd
choose timerlist_timer_duration_current_get() with same semantics of the
function.

4)
an additional function in tlist would be helpful for calculating the
difference in times, such as timerlist_timer_duration_calculate (tv1,
tv2, tv_out)

5)
+	nano_monotonic_hz = ts.tv_sec + 1000000000ULL / (unsigned long
long )ts.
experienced c programmers may be able to read this code if they have a
good understanding of operator precedence.  I'd recommend parens to help
clarify to other maintainers the desired order of precedence.

6)
+	timer->expire_time= timerlist_nano_monotonic_time() + nano_duration;

missing space

7)
regarding comment 3 and 4, this could be simplified throughout the code
base by making a new function in timerlist called

timerlist_nano_current_get() which retrieves the current nanoseconds
from either the epoch or the monotonic clock source depending on os
support.

this gets rid of all of the timersub junk throughout the code.

8)
after reviewing the changes, I'm not convinced the absolute timers work
as expected by the coroapi.

good work

Regards
-steve



On Tue, 2009-07-21 at 15:42 +0200, Jan Friesse wrote:
> Patch should solve problem with ntp and corosync (what it really does)
> by using clock_gettime.
> 
> I tried to find EVERY appearance of gettimeofday and where it makes
> sense (so basically, where this value is used as timer) convert to call
> clock_gettime.
> 
> Patch modifies configure.ac to detect if clock_gettime and
> CLOCK_MONOTONIC exists on system. If not, gettimeofday will be used (so
> you will get old behavior).
> 
> Comments are welcomed.
> 
> Regards,
>   Honza
> _______________________________________________
> Openais mailing list
> Openais at lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/openais