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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 28, 2012, 20:15 UTC
Message-ID: <7vr4qqzsbe.fsf@alter.siamese.dyndns.org>
In-Reply-To: <003301cd81e4$cd68daa0$683a8fe0$@schmitz-digital.de>

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

Show 36 quoted lines

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

Show 22 quoted lines

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

Show 5 quoted lines

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

Thanks.