Pls sanity check my semtimedop(2) implementation

[Mikko Työläjärvi]

On Sun, 13 Jul 2008, Michael B Allen wrote:

> Hi,
>
> Below is a semtimedop(2) implementation that I'm using for FreeBSD. I
> was hoping someone could look it over and tell me if they think the
> implementation is sound.
>
> The code seems to work ok but when stressing the FreeBSD build of my app
> I have managed to provoke errors related to concurrency (usually when a
> SIGALRM goes off). The Linux build works flawlessesly so I'm wondering
> about this one critical function that is different.

At the very least you need to check errno when semop() returns -1.
Unless it is EINTR, you have other problems.

Also, if there is any other code using the timer across this function
call, you have race conditions between changing the signal handler and
setting the timer.  Even if there is no other use of the timer across
this function, resetting the signal handler before disarming the timer
leaves you open to the signal being handled by the default handler
which will make the process exit.

   $.02,
   /Mikko

> Is there any reason why I would want to use ITIMER_VIRTUAL /
> SIGVTALRM instead of ITIMER_REAL / SIGALRM?
>
> Or perhaps I should be using a different implementation entirely?
>
> Mike
>
> int
> _semtimedop(int semid,
>       struct sembuf *array,
>       size_t nops,
>       struct timespec *_timeout)
> {
>   struct timeval timeout, before, after;
>   struct itimerval value, ovalue;
>   struct sigaction sa, osa;
>   int ret;
>
>   if (_timeout) {
>       timeout.tv_sec = _timeout->tv_sec;
>       timeout.tv_usec = _timeout->tv_nsec / 1000;
>
>       if (gettimeofday(&before, NULL) == -1) {
>           return -1;
>       }
>
>       memset(&value, 0, sizeof value);
>       value.it_value = timeout;
>
>       memset(&sa, 0, sizeof sa);
>       /* signal_print writes the signal info to a log file
>        */
>       sa.sa_sigaction = signal_print;
>       sa.sa_flags = SA_SIGINFO;
>       sigemptyset(&sa.sa_mask);
>       sigaction(SIGALRM, &sa, &osa);
>
>       if (setitimer(ITIMER_REAL, &value, &ovalue) == -1) {
>           sigaction(SIGALRM, &osa, NULL);
>           return -1;
>       }
>   }
>
>   ret = semop(semid, array, nops);
>
>   if (_timeout) {
>       sigaction(SIGALRM, &osa, NULL);
>
>       if (setitimer(ITIMER_REAL, &ovalue, NULL) == -1) {
>           return -1;
>       }
>   }
>
>   if (ret == -1) {
>       if (_timeout) {
>           struct timeval elapsed;
>
>           if (gettimeofday(&after, NULL) == -1) {
>               return -1;
>           }
>
>           _timeval_diff(&after, &before, &elapsed);
>
>           if (timercmp(&elapsed, &timeout, >=))
>               errno = EAGAIN;
>       }
>
>       return -1;
>   }
>
>   return 0;
> }
> _______________________________________________
> freebsd-hackers at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "freebsd-hackers-unsubscribe at freebsd.org"
>