"Joachim Schmitz" [off-list ref] writes:
quoted
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.
quoted
quoted
+ 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?
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" [off-list ref] writes:
quoted
quoted
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.
quoted
quoted
quoted
+ 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));
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
quoted
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" [off-list ref] writes:
quoted
quoted
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.
quoted
quoted
quoted
quoted
+ 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
"Joachim Schmitz" [off-list ref] writes:
quoted
quoted
quoted
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.
quoted
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.
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" [off-list ref] writes:
quoted
quoted
quoted
quoted
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.
quoted
quoted
Unless I screwed up the operator precedence?
I think you did, but not in the version we see below.
quoted
int git_setitimer(int which, const struct itimerval *value,
struct itimerval *ovalue)
{
int ret = 0;
if (!value ) {
Style: space before ')'?
Will fix.
quoted
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.
quoted
}
if ( value->it_value.tv_sec < 0
Style: space after ')'?
After '(', I guess? Will fix.
quoted
|| 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.
quoted
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.
quoted
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