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

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

[Joachim Schmitz]

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;
+	}
+	return ret;
+}
+
+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);
+			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;
+	}
+
+	return ret;
+}
-- 
1.7.12

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

[Junio C Hamano]

"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;
	}
	
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?

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()".

> +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).

> +			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.

> +		default: errno = EINVAL; ret = -1;
> +	}
> +
> +	return ret;
> +}

Thanks.

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

[Joachim Schmitz]

> 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

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

[Junio C Hamano]

"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.

>> > +	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?

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

[Joachim Schmitz]

> 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.

> >> > +	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));

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

[Joachim Schmitz]

> From: Joachim Schmitz [mailto:jojo@schmitz-digital.de]
> Sent: Thursday, August 30, 2012 7:23 PM
> To: 'Junio C Hamano'
> Cc: 'git@vger.kernel.org'
> Subject: RE: [PATCH 1/2] Support for setitimer() on platforms lacking it
> 
> > 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.
> 
> > >> > +	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));

OK, here it goes again, not yet as a patch, just plain code for comment:

$ cat itimer.c
/* 
 * Rely on system headers (<sys/time.h>) to contain struct itimerval
 * and git-compat-util.h to have the prototype for git_getitimer().
 * As soon as there's a platform where that is not the case, we'd need
 * an itimer .h.
 */
#include "../git-compat-util.h"

#ifndef NO_GETITIMER /* not yet needed anywhere else in git */
static
#endif
int git_getitimer(int which, struct itimerval *value)
{
	int ret = 0;

	if (!value) {
		errno = EFAULT;
		return -1;
	}

	switch (which) {
	case ITIMER_REAL:
#if 0
		value->it_value.tv_usec = 0;
		value->it_value.tv_sec = alarm(0);
		ret = 0; /* if alarm() fails, we get a SIGLIMIT */
		break;
#else
		/*
		 * As an emulation via alarm(0) won't tell us how many
		 * usecs are left, we don't support it altogether.
		 */
#endif
	case ITIMER_VIRTUAL:
	case ITIMER_PROF:
		errno = ENOTSUP;
		ret = -1;
		break;
	default:
		errno = EINVAL;
		ret = -1;
		break;
	}
	return ret;
}

int git_setitimer(int which, const struct itimerval *value,
				struct itimerval *ovalue)
{
	int ret = 0;

	if (!value ) {
		errno = EFAULT;
		return -1;
	}

	if ( value->it_value.tv_sec < 0
	    || 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() */

	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));
		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;
}

Would this pass muster? The previous version had a bug too, of ovalue was !NULL the switch was never reached.

Bye, Jojo

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

[Junio C Hamano]

"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 ;-)

The emulation in mingw.c 6072fc3 (Windows: Implement setitimer() and
sigaction()., 2007-11-13) may want to be tightened in a similar way.

> 	}
>
> 	if ( value->it_value.tv_sec < 0

Style: space after ')'?

> 	    || 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 to

	if (ovalue) {
        	errno = ENOTSUP;
                return -1;
	}

which is how I understood what "the latter" in the paragraph I
quoted from you above meant.

> 	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.

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

[Joachim Schmitz]

> 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 ')'?

Will fix.
 
> > 		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.

> > 	}
> >
> > 	if ( value->it_value.tv_sec < 0
> 
> Style: space after ')'?

After '(', I guess? Will fix.
 
> > 	    || 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 to

True.
 
> 	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.
 
> > 	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

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

[Johannes Sixt]

Am 03.09.2012 11:31, schrieb Joachim Schmitz:
> 
> 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.

The error message is really just a reminder that the implementation is
not complete. Writing it here has the advantage that it is much more
accurate than a generic "invalid argument" or "operation not supported"
error that the caller would be able to write.

-- Hannes

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 03.09.2012 11:31, schrieb Joachim Schmitz:
>> 
>> 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.
>
> The error message is really just a reminder that the implementation is
> not complete. Writing it here has the advantage that it is much more
> accurate than a generic "invalid argument" or "operation not supported"
> error that the caller would be able to write.

Joachim quoted irrelevant (to you) part and made comments on it, but
the issue I raised by Ccing you was about diagnosing NULL passed in
newvalue parameter, which Joachim's code did like this:

    > int git_setitimer(int which, const struct itimerval *value,
    > 				struct itimerval *ovalue)
    > {
    > 	int ret = 0;
    >
    > 	if (!value ) {
    > 		errno = EFAULT;
    > 		return -1;

    EFAULT is good ;-)

    The emulation in mingw.c 6072fc3 (Windows: Implement setitimer() and
    sigaction()., 2007-11-13) may want to be tightened in a similar way.

but mingw.c doesn't seem to.

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

[Junio C Hamano]

"Joachim Schmitz" <jojo@schmitz-digital.de> writes:

>> > 	if (!value ) {
>> 
>> Style: space before ')'?
>
> Will fix.
>  
>> > 		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.

I don't care too deeply either way.  The above was not a comment
meant for you, but was to point out the error checking when the
newvalue is NULL---it is missing in mingw.c and I think the
condition should be checked.

> On top here SA_RESTART is used, which is not available in HP
> NonStop (so I have a "-DSA_RESTART=0" in COMPAT_CFLAGS).

If you cannot re-trigger the timer, then you will see "20%" shown
after one second, silence for 4 seconds and then "done", for an
operation that takes 5 seconds.  Which is not the end of the world,
though.  It does not affect correctness.

The other use of itimer in our codebase is the early-output timer,
but that also is about perceived latency, and not about correctness,
so it is possible that you do not have to support anything (i.e. not
even setting an alarm) at all.

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

[Joachim Schmitz]

> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Monday, September 03, 2012 9:03 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:
> 
> >> > 	if (!value ) {
> >>
> >> Style: space before ')'?
> >
> > Will fix.
> >
> >> > 		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.
> 
> I don't care too deeply either way.  The above was not a comment
> meant for you, but was to point out the error checking when the
> newvalue is NULL---it is missing in mingw.c and I think the
> condition should be checked.

 Ah, OK. Guess Johannes and I misunderstood ;-)

> > On top here SA_RESTART is used, which is not available in HP
> > NonStop (so I have a "-DSA_RESTART=0" in COMPAT_CFLAGS).
> 
> If you cannot re-trigger the timer, then you will see "20%" shown
> after one second, silence for 4 seconds and then "done", for an
> operation that takes 5 seconds.  Which is not the end of the world,
> though.  It does not affect correctness.

That does seem to work, if I do e.g. a "git clone" on git itself (being a fairly large repository), I see it updating the % values
about once per second.

> The other use of itimer in our codebase is the early-output timer,
> but that also is about perceived latency, and not about correctness,
> so it is possible that you do not have to support anything (i.e. not
> even setting an alarm) at all.

OK, I'll go for that one-liner in git-compat-utils.h then

#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

It certainly seems to work just fine for me.
Could as well be #ifdef __TANDEM, I won't mind.

Bye, Jojo

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

[Junio C Hamano]

"Joachim Schmitz" <jojo@schmitz-digital.de> writes:

>> If you cannot re-trigger the timer, then you will see "20%" shown
>> after one second, silence for 4 seconds and then "done", for an
>> operation that takes 5 seconds.  Which is not the end of the world,
>> though.  It does not affect correctness.
>
> That does seem to work, if I do e.g. a "git clone" on git itself
> (being a fairly large repository), I see it updating the % values
> about once per second.

Ehh, so somebody is re-arming the alarm().  I am not sure where,
though.

 ... thinks for a while, then a lightbulb slowly starts to glow ...

Where are you cloning from, and does the other side of the clone
(i.e. upload-pack) also run on your tandem port?  If you are cloning
from one of my public distribution points (e.g. k.org, repo.or.cz,
or github.com), then I think the progress indicator you are seeing
is coming from the other side, not generated by your local timer.

Only with the observation of "clone", I cannot tell if your timer is
working.  You can try repacking the test repository you created by
your earlier "git clone" with "git repack -a -d -f" and see what
happens.

> OK, I'll go for that one-liner in git-compat-utils.h then
>
> #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
>
> It certainly seems to work just fine for me.

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

[Joachim Schmitz]

> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Tuesday, September 04, 2012 6:58 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:
> 
> >> If you cannot re-trigger the timer, then you will see "20%" shown
> >> after one second, silence for 4 seconds and then "done", for an
> >> operation that takes 5 seconds.  Which is not the end of the world,
> >> though.  It does not affect correctness.
> >
> > That does seem to work, if I do e.g. a "git clone" on git itself
> > (being a fairly large repository), I see it updating the % values
> > about once per second.
> 
> Ehh, so somebody is re-arming the alarm().  I am not sure where,
> though.
> 
>  ... thinks for a while, then a lightbulb slowly starts to glow ...
> 
> Where are you cloning from, and does the other side of the clone
> (i.e. upload-pack) also run on your tandem port?  If you are cloning
> from one of my public distribution points (e.g. k.org, repo.or.cz,
> or github.com), then I think the progress indicator you are seeing
> is coming from the other side, not generated by your local timer.

I used GutHub
The cloning from NonStop doesn't work at all, different story, but looks like poll isn#t working.
Not poll's fault tough, but on out plaftom ssh (non-interactive) give a pipe rather than a socket and recv(...MSG_PEEK) then fails
with ENOTSOCK

> Only with the observation of "clone", I cannot tell if your timer is
> working.  You can try repacking the test repository you created by
> your earlier "git clone" with "git repack -a -d -f" and see what
> happens.

It does update the counter too.

> > OK, I'll go for that one-liner in git-compat-utils.h then
> >
> > #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
> >
> > It certainly seems to work just fine for me.

Bye, Jojo

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

[Junio C Hamano]

"Joachim Schmitz" <jojo@schmitz-digital.de> writes:

>> Only with the observation of "clone", I cannot tell if your timer is
>> working.  You can try repacking the test repository you created by
>> your earlier "git clone" with "git repack -a -d -f" and see what
>> happens.
>
> It does update the counter too.

Yeah, that was not a very good way to diagnose it.

You see the progress from pack-objects (which is the underlying
machinery "git repack" uses) only because it knows how many objects
it is going to pack, and it updates the progress meter for every
per-cent progress it makes, without any help from the timer
interrupt.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Joachim Schmitz" <jojo@schmitz-digital.de> writes:
>
>>> Only with the observation of "clone", I cannot tell if your timer is
>>> working.  You can try repacking the test repository you created by
>>> your earlier "git clone" with "git repack -a -d -f" and see what
>>> happens.
>>
>> It does update the counter too.
>
> Yeah, that was not a very good way to diagnose it.
>
> You see the progress from pack-objects (which is the underlying
> machinery "git repack" uses) only because it knows how many objects
> it is going to pack, and it updates the progress meter for every
> per-cent progress it makes, without any help from the timer
> interrupt.

I think the "Counting objects: $number" phase is purely driven by
the timer, as there is no way to say "we are done X per-cent so
far".

Doesn't your repack show "Counting objects: " with a number once,
pause forever and then show "Counting objects: $number, done."?

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

[Johannes Sixt]

Am 04.09.2012 19:23, schrieb Joachim Schmitz:
>> From: Junio C Hamano [mailto:gitster@pobox.com]
>> Only with the observation of "clone", I cannot tell if your timer is
>> working.  You can try repacking the test repository you created by
>> your earlier "git clone" with "git repack -a -d -f" and see what
>> happens.
> 
> It does update the counter too.

Last time I looked at the progress code, it updated the progress text
also every time the percent value changed. If you have a large count or
large objects such that it takes more than a second to increase the
percentage, than you don't get a smooth progress. Nor do you for phases
that do not have a percentage, like the "counting objects" phase of
pack-objects.

-- Hannes

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

[Joachim Schmitz]

> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Tuesday, September 04, 2012 8:47 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org; 'Johannes Sixt'
> Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > "Joachim Schmitz" <jojo@schmitz-digital.de> writes:
> >
> >>> Only with the observation of "clone", I cannot tell if your timer is
> >>> working.  You can try repacking the test repository you created by
> >>> your earlier "git clone" with "git repack -a -d -f" and see what
> >>> happens.
> >>
> >> It does update the counter too.
> >
> > Yeah, that was not a very good way to diagnose it.
> >
> > You see the progress from pack-objects (which is the underlying
> > machinery "git repack" uses) only because it knows how many objects
> > it is going to pack, and it updates the progress meter for every
> > per-cent progress it makes, without any help from the timer
> > interrupt.
> 
> I think the "Counting objects: $number" phase is purely driven by
> the timer, as there is no way to say "we are done X per-cent so
> far".
> 
> Doesn't your repack show "Counting objects: " with a number once,
> pause forever and then show "Counting objects: $number, done."?

Yes, only once, when it is done
$ ./git repack -a -d -f
warning: no threads support, ignoring --threads
Counting objects: 140302, done.
Compressing objects:   1% (1385/138407)

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

[Junio C Hamano]

"Joachim Schmitz" <jojo@schmitz-digital.de> writes:

>> From: Junio C Hamano [mailto:gitster@pobox.com]
>> Sent: Tuesday, September 04, 2012 8:47 PM
>> To: Joachim Schmitz
>> Cc: git@vger.kernel.org; 'Johannes Sixt'
>> Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it
>> 
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> > "Joachim Schmitz" <jojo@schmitz-digital.de> writes:
>> >
>> >>> Only with the observation of "clone", I cannot tell if your timer is
>> >>> working.  You can try repacking the test repository you created by
>> >>> your earlier "git clone" with "git repack -a -d -f" and see what
>> >>> happens.
>> >>
>> >> It does update the counter too.
>> >
>> > Yeah, that was not a very good way to diagnose it.
>> >
>> > You see the progress from pack-objects (which is the underlying
>> > machinery "git repack" uses) only because it knows how many objects
>> > it is going to pack, and it updates the progress meter for every
>> > per-cent progress it makes, without any help from the timer
>> > interrupt.
>> 
>> I think the "Counting objects: $number" phase is purely driven by
>> the timer, as there is no way to say "we are done X per-cent so
>> far".
>> 
>> Doesn't your repack show "Counting objects: " with a number once,
>> pause forever and then show "Counting objects: $number, done."?
>
> Yes, only once, when it is done
> $ ./git repack -a -d -f
> warning: no threads support, ignoring --threads
> Counting objects: 140302, done.
> Compressing objects:   1% (1385/138407)

So this strongly suggests that (1) your "poor-man's" is not a real
substitute for recurring itimer, and (2) users could live with the
progress.c code without any itimer firing.

Perhaps a no-op macro would work equally well?

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

[Joachim Schmitz]

> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Wednesday, September 05, 2012 12:45 AM
> 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:
> 
> >> From: Junio C Hamano [mailto:gitster@pobox.com]
> >> Sent: Tuesday, September 04, 2012 8:47 PM
> >> To: Joachim Schmitz
> >> Cc: git@vger.kernel.org; 'Johannes Sixt'
> >> Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it
> >>
> >> Junio C Hamano <gitster@pobox.com> writes:
> >>
> >> > "Joachim Schmitz" <jojo@schmitz-digital.de> writes:
> >> >
> >> >>> Only with the observation of "clone", I cannot tell if your timer is
> >> >>> working.  You can try repacking the test repository you created by
> >> >>> your earlier "git clone" with "git repack -a -d -f" and see what
> >> >>> happens.
> >> >>
> >> >> It does update the counter too.
> >> >
> >> > Yeah, that was not a very good way to diagnose it.
> >> >
> >> > You see the progress from pack-objects (which is the underlying
> >> > machinery "git repack" uses) only because it knows how many objects
> >> > it is going to pack, and it updates the progress meter for every
> >> > per-cent progress it makes, without any help from the timer
> >> > interrupt.
> >>
> >> I think the "Counting objects: $number" phase is purely driven by
> >> the timer, as there is no way to say "we are done X per-cent so
> >> far".
> >>
> >> Doesn't your repack show "Counting objects: " with a number once,
> >> pause forever and then show "Counting objects: $number, done."?
> >
> > Yes, only once, when it is done
> > $ ./git repack -a -d -f
> > warning: no threads support, ignoring --threads
> > Counting objects: 140302, done.
> > Compressing objects:   1% (1385/138407)
> 
> So this strongly suggests that (1) your "poor-man's" is not a real
> substitute for recurring itimer, and (2) users could live with the
> progress.c code without any itimer firing.

OK

> Perhaps a no-op macro would work equally well?

Like the following:

diff --git a/git-compat-util.h b/git-compat-util.h
index 18089f0..55b9421 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h@@ -163,6 +163,10 @@
 #define probe_utf8_pathname_composition(a,b)
 #endif

+#ifdef NO_SETITIMER
+#define setitimer(w,v,o) /* NOP */
+#endif
+
 #ifdef MKDIR_WO_TRAILING_SLASH
 #define mkdir(a,b) compat_mkdir_wo_trailing_slash((a),(b))
 extern int compat_mkdir_wo_trailing_slash(const char*, mode_t);

Does work for me and does not seem to make any difference, not in those test cases at least

Does the inability to re-arm the timer depend on SA_RESTART, possibly?
If so we may instead want 
#if SA_RSTART == 0  && defined(NO_SETITIMER)

Bye, Jojo