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

["Joachim Schmitz" <jojo@schmitz-digital.de>]

> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Tuesday, August 28, 2012 10:16 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it
> 
> "Joachim Schmitz" <jojo@schmitz-digital.de> writes:
> 
> > Implementation includes getitimer(), but for now it is static.
> > Supports ITIMER_REAL only.
> >
> > Signed-off-by: Joachim Schmitz <jojo@schmitz-digital.de>
> > ---
> > 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