RE: [PATCH 1/2] Support for setitimer() on platforms lacking it
From: Junio C Hamano [mailto:gits...@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 j...@schmitz-digital.de writes: From: Junio C Hamano [mailto:gits...@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 gits...@pobox.com writes: Joachim Schmitz j...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Support for setitimer() on platforms lacking it
Joachim Schmitz j...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Support for setitimer() on platforms lacking it
Junio C Hamano gits...@pobox.com writes: Joachim Schmitz j...@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.? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Support for setitimer() on platforms lacking it
Am 04.09.2012 19:23, schrieb Joachim Schmitz: From: Junio C Hamano [mailto:gits...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] Support for setitimer() on platforms lacking it
From: Junio C Hamano [mailto:gits...@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 gits...@pobox.com writes: Joachim Schmitz j...@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) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Support for setitimer() on platforms lacking it
Joachim Schmitz j...@schmitz-digital.de writes: From: Junio C Hamano [mailto:gits...@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 gits...@pobox.com writes: Joachim Schmitz j...@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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] Support for setitimer() on platforms lacking it
From: Junio C Hamano [mailto:gits...@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 j...@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 100
|| 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 = 50;
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
Re: [PATCH 1/2] Support for setitimer() on platforms lacking it
Johannes Sixt j...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Support for setitimer() on platforms lacking it
Joachim Schmitz j...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] Support for setitimer() on platforms lacking it
From: Junio C Hamano [mailto:gits...@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 j...@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_usec0))
#endif
It certainly seems to work just fine for me.
Could as well be #ifdef __TANDEM, I won't mind.
Bye, Jojo
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] Support for setitimer() on platforms lacking it
From: Joachim Schmitz [mailto:j...@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:gits...@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 j...@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 100
|| 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
RE: [PATCH 1/2] Support for setitimer() on platforms lacking it
From: Junio C Hamano [mailto:gits...@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 j...@schmitz-digital.de writes:
Implementation includes getitimer(), but for now it is static.
Supports ITIMER_REAL only.
Signed-off-by: Joachim Schmitz j...@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 000..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 100
+ || 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
Re: [PATCH 1/2] Support for setitimer() on platforms lacking it
Joachim Schmitz j...@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?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Support for setitimer() on platforms lacking it
Joachim Schmitz j...@schmitz-digital.de writes:
Implementation includes getitimer(), but for now it is static.
Supports ITIMER_REAL only.
Signed-off-by: Joachim Schmitz j...@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 000..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 100
+ || 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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] Support for setitimer() on platforms lacking it
Implementation includes getitimer(), but for now it is static.
Supports ITIMER_REAL only.
Signed-off-by: Joachim Schmitz j...@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 000..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 100
+ || 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
