> From: Junio C Hamano [mailto:gitster@xxxxxxxxx] > Sent: Tuesday, August 28, 2012 10:16 PM > To: Joachim Schmitz > Cc: git@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it > > "Joachim Schmitz" <jojo@xxxxxxxxxxxxxxxxxx> writes: > > > Implementation includes getitimer(), but for now it is static. > > Supports ITIMER_REAL only. > > > > Signed-off-by: Joachim Schmitz <jojo@xxxxxxxxxxxxxxxxxx> > > --- > > May need a header file for ITIMER_*, struct itimerval and the prototypes, > > But for now, and the HP NonStop platform this isn't needed, here > > <sys/time> has ITIMER_* and struct timeval, and the prototypes can > > vo into git-compat-util.h for now (Patch 2/2) > > > > compat/itimer.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 50 insertions(+) > > create mode 100644 compat/itimer.c > > > > diff --git a/compat/itimer.c b/compat/itimer.c > > new file mode 100644 > > index 0000000..713f1ff > > --- /dev/null > > +++ b/compat/itimer.c > > @@ -0,0 +1,50 @@ > > +#include "../git-compat-util.h" > > + > > +static int git_getitimer(int which, struct itimerval *value) > > +{ > > + int ret = 0; > > + > > + switch (which) { > > + case ITIMER_REAL: > > + value->it_value.tv_usec = 0; > > + value->it_value.tv_sec = alarm(0); > > + ret = 0; /* if alarm() fails, we get a SIGLIMIT */ > > + break; > > + case ITIMER_VIRTUAL: /* FALLTHRU */ > > + case ITIMER_PROF: errno = ENOTSUP; ret = -1; break; > > + default: errno = EINVAL; ret = -1; > > + } > > Just a style thing, but we align case arms and switch statements, > like this: > > switch (which) { > case ...: > stmt; > break; > default: > stmt; > break; > } OK, I'll fix the syle > Because alarm() runs in integral seconds granularity, this could > return 0.0 sec (i.e. "do not re-trigger this alarm any more") in > ovalue after setting alarm(1) (via git_setitimer()) and calling this > function (via git_setitimer() again) before the timer expires, no? > Is it a desired behaviour? Unintentional, never really thought about this. > What I am most worried about is that callers _might_ take this > emulation too seriously, grab the remainder from getitimer(), and > drives a future call to getitimer() with the returned value, and > accidentally cause the "recurring" nature of the request to be > disabled. > > 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? > > +int git_setitimer(int which, const struct itimerval *value, > > + struct itimerval *ovalue) > > +{ > > + int ret = 0; > > + > > + if (!value > > + || value->it_value.tv_usec < 0 > > + || value->it_value.tv_usec > 1000000 > > + || value->it_value.tv_sec < 0) { > > + errno = EINVAL; > > + return -1; > > + } > > + > > + else if (ovalue) > > + if (!git_getitimer(which, ovalue)) > > + return -1; /* errno set in git_getitimer() */ > > + > > + else > > + 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? > > + ret = 0; /* if alarm() fails, we get a SIGLIMIT */ > > + break; > > + case ITIMER_VIRTUAL: /* FALLTHRU */ > > + case ITIMER_PROF: errno = ENOTSUP; ret = -1; break; > > Please don't add a misleading "fallthru" label here. We do not say > "fallthru" when "two case arms do _exactly_ the same thing". Only > when the one arm does some pre-action before the common action, i.e. > > switch (which) { > case one: > do some thing specific to one; > /* fallthru */ > case two: > do some thing common between one and two; > break; > } > > we label it "fallthru" to make it clear to the readers that it is > not "missing a break" but is deliberate. I'll fix those too. > > + default: errno = EINVAL; ret = -1; > > + } > > + > > + return ret; > > +} > > Thanks. Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html