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

Will fix.
 

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

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.

After '(', I guess? Will fix.
 

True.
 

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

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