svn commit: r236909 - head/sbin/hastd

[Hans Petter Selasky]

On Tuesday 12 June 2012 05:49:33 Bruce Evans wrote:
> On Mon, 11 Jun 2012, Hans Petter Selasky wrote:
> > On Monday 11 June 2012 22:21:51 Hans Petter Selasky wrote:
> >> On Monday 11 June 2012 22:05:07 Pawel Jakub Dawidek wrote:
> >>> On Mon, Jun 11, 2012 at 07:21:00PM +0000, Hans Petter Selasky wrote:
> >>>> Author: hselasky
> >>>> Date: Mon Jun 11 19:20:59 2012
> >>>> New Revision: 236909
> >>>> URL: http://svn.freebsd.org/changeset/base/236909
> >>>> 
> 

Hi,

> The point is in 2038, on systems with 32-bit signed time_t's.  None
> should exist then.  Even if time_t is 32 bits, it can be unsigned, and
> never become negative, and work until 2106.  Changing time_t from
> signed to unsigned would break mainly times before the Epoch, which
> are invalid for current times anyway, and expose broken code which
> assumes that time_t is signed.

Lets assume you need to reboot the system at some point and that solves the 
problem.

> 
> > functionality stop working then, because pthread_cond_timedwait() has a
> > check for negative time?
> 
> This check is just to prevent invalid times.  It does prevents hacks like
> the kernel treating negative times as large unsigned ones so that the
> result is much the same as changing time_t to unsigned, without actually
> changing time_t.
> 
> > Or is hastd wrong, that it can compute a timeout offset which is outside
> > the valid range, because it uses a simple:
> > 
> > tv_sec += timeout?
> > tv_sec %= 1000000000; /* Is this perhaps missing in hastd and other
> > drivers ???? */
> > 
> > What is the modulus which should be used for tv_sec?
> 
> `tv_sec %= ANY' makes no sense.
> 
> With CLOCK_MONOTONIC, signed 32-bit time_t's work for 86 years after the
> unspecified point in the past that CLOCK_MONOTONIC is relative to.  This
> should be enough for anyone, provided the unspecified point is the boot
> time.  hastd also uses mostly-relative, mostly-monotonic, often 32-bit
> signed in timeouts internally.  E.g., in the function that seems to be
> causing problems:
> 

When CLOCK_REALTIME finally goes negative, then pthread_cond_timedwait() will 
simply return an error code, due to the negative tv_sec check in there! I see 
other clients like sendmail, using even simpler formats like:

tv_nsec = 0;
tv_sec = time(NULL);

If this piece of code stops working at a given data, regardless of uptime, 
shouldn't that be fixed now?

> % static __inline bool
> % cv_timedwait(pthread_cond_t *cv, pthread_mutex_t *lock, int timeout)
> % {
> % 	struct timespec ts;
> % 	int error;
> %
> % 	if (timeout == 0) {
> % 		cv_wait(cv, lock);
> % 		return (false);
> % 	}
> %
> %         error = clock_gettime(CLOCK_MONOTONIC, &ts);
> 
> Style bug (corrupt tab in Oct 22 2011 version).

I think I fixed this style bug when reverting.

> 
> % 	PJDLOG_ASSERT(error == 0);
> % 	ts.tv_sec += timeout;
> 
> This converts to absolute monotonic time.  Even a 16-bit signed int is
> probably enough for `timeout'.
> 
> % 	error = pthread_cond_timedwait(cv, lock, &ts);
> % 	PJDLOG_ASSERT(error == 0 || error == ETIMEDOUT);
> % 	return (error == ETIMEDOUT);
> % }
> 
> I don't see any bugs here.

Thanks for feedback Bruce.

A final question on the matter:

I tried to use CLOCK_MONOTONIC_FAST when setting up the condition variable. 
That did not appear to be support in 9-stable. Is there a list of supported 
clocks anywhere?

--HPS