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

From: Junio C Hamano [mailto:gitster@pobox.com]
Sent: Sunday, September 02, 2012 10:44 PM
To: Joachim Schmitz
Cc: git@vger.kernel.org; Johannes Sixt
Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it

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

quoted

quoted

quoted

quoted

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.

Thanks.

quoted

quoted

Unless I screwed up the operator precedence?

I think you did, but not in the version we see below.

quoted

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

	if (!value ) {

Style: space before ')'?

Will fix.
 

quoted

		errno = EFAULT;
		return -1;

EFAULT is good ;-)

That's what 'man setitimer()' on Linux says to happen if invalid value is found.
 

The emulation in mingw.c 6072fc3 (Windows: Implement setitimer() and
sigaction()., 2007-11-13) may want to be tightened in a similar way.

Hmm, I see that there the errors are handled differently, like this:

        if (ovalue != NULL)
                return errno = EINVAL,
                        error("setitimer param 3 != NULL not implemented");

Should this be done in my setitimer() too? Or rather be left to the caller?
I tend to the later.

quoted

	}

	if ( value->it_value.tv_sec < 0

Style: space after ')'?

After '(', I guess? Will fix.
 

quoted

	    || 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() */

As nobody passes non-NULL ovalue to setitimer(), I think we should
instead get rid of git_getitmier() implemenation, and change this to

True.
 

	if (ovalue) {
        	errno = ENOTSUP;
                return -1;
	}

which is how I understood what "the latter" in the paragraph I
quoted from you above meant.

OK, will do this and then I'll rename the entire file into getitimer.c.
 

quoted

	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));

OK.

quoted

		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;
}

Other than that, looks good.

Thanks.

I had a closer look at the places in git where setitimer() is used. It is in 2 files, progress.c and builtin/log.c.
In progress.c :
static void set_progress_signal(void)
{
        struct sigaction sa;
        struct itimerval v;

        progress_update = 0;

        memset(&sa, 0, sizeof(sa));
        sa.sa_handler = progress_interval;
        sigemptyset(&sa.sa_mask);
        sa.sa_flags = SA_RESTART;
        sigaction(SIGALRM, &sa, NULL);

        v.it_interval.tv_sec = 1;
        v.it_interval.tv_usec = 0;
        v.it_value = v.it_interval;
        setitimer(ITIMER_REAL, &v, NULL);
}

static void clear_progress_signal(void)
{
        struct itimerval v = {{0,},};
        setitimer(ITIMER_REAL, &v, NULL);
        signal(SIGALRM, SIG_IGN);
        progress_update = 0;
}

So it uses a 1 sec timeout, which is a good match to my implementation, but also uses it_interval, meant to 're-arm' the timer after
it expired.
My implementation doesn't do that at all, and I also don't see how it possibly could (short of installing a signal handler, which
then conflicts with the one use in progress.c).
On top here SA_RESTART is used, which is not available in HP NonStop (so I have a "-DSA_RESTART=0" in COMPAT_CFLAGS).

In builtin/log.c it doesn't use it_interval, which is a good match to my implementation, but uses 1/2 a sec and 1/10 sec, so here
would be a victim of a 1 sec upgrade. This is probably acceptable.
...
         * NOTE! We don't use "it_interval", because if the
         * reader isn't listening, we want our output to be
         * throttled by the writing, and not have the timer
         * trigger every second even if we're blocked on a
         * reader!
         */
        early_output_timer.it_value.tv_sec = 0;
        early_output_timer.it_value.tv_usec = 500000;
        setitimer(ITIMER_REAL, &early_output_timer, NULL);
...
static void setup_early_output(struct rev_info *rev)
{
        struct sigaction sa;

        /*
         * Set up the signal handler, minimally intrusively:
         * we only set a single volatile integer word (not
         * using sigatomic_t - trying to avoid unnecessary
         * system dependencies and headers), and using
         * SA_RESTART.
         */
        memset(&sa, 0, sizeof(sa));
        sa.sa_handler = early_output;
        sigemptyset(&sa.sa_mask);
        sa.sa_flags = SA_RESTART;
        sigaction(SIGALRM, &sa, NULL);

        /*
         * If we can get the whole output in less than a
         * tenth of a second, don't even bother doing the
         * early-output thing..
         *
         * This is a one-time-only trigger.
         */
        early_output_timer.it_value.tv_sec = 0;
        early_output_timer.it_value.tv_usec = 100000;
        setitimer(ITIMER_REAL, &early_output_timer, NULL);
}

static void finish_early_output(struct rev_info *rev)
{
        int n = estimate_commit_count(rev, rev->commits);
        signal(SIGALRM, SIG_IGN);
        show_early_header(rev, "done", n);
}

This means, however, at least to my understanding, that my setitimer() basically degrades to an 'alarm(1);' resp. 'alarm(0);', so
could possibly be simplified to:

#ifdef NO_SETITIMER /* poor man's setitimer() */
#define setitimer(w,v,o) alarm((v)->it_value.tv_sec+((v)->it_value.tv_usec>0))
#endif

in e.g. git-compat-util.h

Opinions?

Bye, Jojo