Pls sanity check my semtimedop(2) implementation

[Michael B Allen]

On 7/13/08, Mikko Työläjärvi <mbsd at pacbell.net> wrote:
> 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.

Hi Mikko,

So if some other code uses setitimer(2) for whatever reason, then I
have the potential for a race. I'm not aware of any other such
instances of setitimer but my app is actually a plugin for a larger
application so I can't entirely rule out the possibility.

Is there any facility for creating a stateful timer so that I don't
run into this problem?

Can anyone provide the basis for an alternative implementation?

Should I use select(2) instead?

Mike

> > 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;
> > }