RE: [PATCH 1/2] Support for setitimer() on platforms lacking it
Show 30 quoted lines
> 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" <jojo@schmitz-digital.de> writes:
>
> >> > > 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.
>
> >> Unless I screwed up the operator precedence?
>
> I think you did, but not in the version we see below.
>
> > int git_setitimer(int which, const struct itimerval *value,
> > struct itimerval *ovalue)
> > {
> > int ret = 0;
> >
> > if (!value ) {
>
> Style: space before ')'?> > 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.
Show 5 quoted lines
> > }
> >
> > if ( value->it_value.tv_sec < 0
>
> Style: space after ')'?
After '(', I guess? Will fix.
Show 11 quoted lines
> > || 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 toShow 7 quoted lines
> 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.
Show 26 quoted lines
> > 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.
>
> > 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
Joachim Schmitz
