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

From: Joachim Schmitz <jojo@schmitz-digital.de>
Date: Aug 30, 2012, 17:22 UTC
Message-ID: <003101cd86d4$14b494a0$3e1dbde0$@schmitz-digital.de>
In-Reply-To: <7vfw74s3oy.fsf@alter.siamese.dyndns.org>

Show 21 quoted lines

> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Thursday, August 30, 2012 7:14 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:
> 
> >> 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?
> 
> I was alluding to the latter.

OK, will do that then.

Show 16 quoted lines

> >> > +	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?
> 
> Can a caller use setitimer to be notified in 5 seconds?

Yes, by setting tv_sec to 5 and tv_usec to 0, or be setting tv_sec to 4 and tv_usec to something > 0.

Unless I screwed up the operator precedence? To make it clearer (any possibly correct?):

	switch (which) {
		case ITIMER_REAL:
			alarm(value->it_value.tv_sec +
				((value->it_value.tv_usec > 0) ? 1 : 0));

Or even just
	switch (which) {
		case ITIMER_REAL:
			alarm(value->it_value.tv_sec + (value->it_value.tv_usec > 0));