"Joachim Schmitz" <jojo@xxxxxxxxxxxxxxxxxx> writes:
> Implementation includes getitimer(), but for now it is static.
> Supports ITIMER_REAL only.
>
> Signed-off-by: Joachim Schmitz <jojo@xxxxxxxxxxxxxxxxxx>
> ---
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html