RE: [PATCH 1/2] Support for setitimer() on platforms lacking it

From: Joachim Schmitz [mailto:jojo@schmitz-digital.de]
Sent: Thursday, August 30, 2012 7:23 PM
To: 'Junio C Hamano'
Cc: 'git@vger.kernel.org'
Subject: RE: [PATCH 1/2] Support for setitimer() on platforms lacking it

quoted

From: Junio C Hamano [mailto:gitster@pobox.com]
Sent: Thursday, August 30, 2012 7:14 PM
To: Joachim Schmitz
Cc: git@vger.kernel.org
Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it

"Joachim Schmitz" [off-list ref] writes:

quoted

quoted

I see no existing code calls setitimer() with non-NULL ovalue, and I
do not think we would add a new caller that would do so in any time
soon, so it may not be a bad idea to drop support of returning the
remaining timer altogether from this emulation layer (just like
giving anything other than ITIMER_REAL gives us ENOTSUP).  That
would sidestep the whole "we cannot answer how many milliseconds are
still remaining on the timer when using emulation based on alarm()".

Should we leave tv_usec untouched then? That was we round up on
the next (and subsequent?) round(s). Or just set to ENOTSUP in
setitimer if ovalue is !NULL?

I was alluding to the latter.

OK, will do that then.

quoted

quoted

quoted

quoted

+	switch (which) {
+		case ITIMER_REAL:
+			alarm(value->it_value.tv_sec +
+				(value->it_value.tv_usec > 0) ? 1 : 0);

Why is this capped to 1 second?  Is this because no existing code
uses the timer for anything other than 1 second or shorter?  If that
is the case, that needs at least some documenting (or a possibly
support for longer expiration, if it is not too cumbersome to add).

As you mention alarm() has only seconds resolution. It is tv_sec
plus 1 if there are tv_usecs > 0, it is rounding up, so we don't
cancel the alarm() if tv_sec is 0 but tv_usec is not. Looks OK to
me?

Can a caller use setitimer to be notified in 5 seconds?

Yes, by setting tv_sec to 5 and tv_usec to 0, or be setting tv_sec to 4 and tv_usec to something > 0.

Unless I screwed up the operator precedence?
To make it clearer (any possibly correct?):

	switch (which) {
		case ITIMER_REAL:
			alarm(value->it_value.tv_sec +
				((value->it_value.tv_usec > 0) ? 1 : 0));

Or even just
	switch (which) {
		case ITIMER_REAL:
			alarm(value->it_value.tv_sec + (value->it_value.tv_usec > 0));

OK, here it goes again, not yet as a patch, just plain code for comment:

$ cat itimer.c
/* 
 * Rely on system headers (<sys/time.h>) to contain struct itimerval
 * and git-compat-util.h to have the prototype for git_getitimer().
 * As soon as there's a platform where that is not the case, we'd need
 * an itimer .h.
 */
#include "../git-compat-util.h"

#ifndef NO_GETITIMER /* not yet needed anywhere else in git */
static
#endif
int git_getitimer(int which, struct itimerval *value)
{
	int ret = 0;

	if (!value) {
		errno = EFAULT;
		return -1;
	}

	switch (which) {
	case ITIMER_REAL:
#if 0
		value->it_value.tv_usec = 0;
		value->it_value.tv_sec = alarm(0);
		ret = 0; /* if alarm() fails, we get a SIGLIMIT */
		break;
#else
		/*
		 * As an emulation via alarm(0) won't tell us how many
		 * usecs are left, we don't support it altogether.
		 */
#endif
	case ITIMER_VIRTUAL:
	case ITIMER_PROF:
		errno = ENOTSUP;
		ret = -1;
		break;
	default:
		errno = EINVAL;
		ret = -1;
		break;
	}
	return ret;
}

int git_setitimer(int which, const struct itimerval *value,
				struct itimerval *ovalue)
{
	int ret = 0;

	if (!value ) {
		errno = EFAULT;
		return -1;
	}

	if ( value->it_value.tv_sec < 0
	    || value->it_value.tv_usec > 1000000
	    || value->it_value.tv_usec < 0) {
		errno = EINVAL;
		return -1;
	}

	if ((ovalue) && (git_getitimer(which, ovalue) == -1))
		return -1; /* errno set in git_getitimer() */

	switch (which) {
	case ITIMER_REAL:
		 /* If tv_usec is > 0, round up to next full sec */
		alarm(value->it_value.tv_sec + (value->it_value.tv_usec > 0));
		ret = 0; /* if alarm() fails, we get a SIGLIMIT */
		break;
	case ITIMER_VIRTUAL:
		case ITIMER_PROF:
		errno = ENOTSUP;
		ret = -1;
		break;
	default:
		errno = EINVAL;
		ret = -1;
		break;
	}

	return ret;
}

Would this pass muster? The previous version had a bug too, of ovalue was !NULL the switch was never reached.

Bye, Jojo