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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 2, 2012, 20:43 UTC
Message-ID: <7vpq64f935.fsf@alter.siamese.dyndns.org>
In-Reply-To: <003d01cd8827$34e90180$9ebb0480$@schmitz-digital.de>

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

Show 7 quoted lines

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

Show 6 quoted lines

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

Show 8 quoted lines

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

Show 15 quoted lines

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