Re: [PATCH] Remove signal handling for nanosleep (bug 16364)

[Florian Weimer <fweimer at redhat dot com>]

On 11/09/2015 03:07 PM, Adhemerval Zanella wrote:

>> Could you write a glibc test case for this?  I don't feel comfortable
>> with removing the workaround without a test, and I can't find an
>> existing one.
> 
> What about:
> 
> 	[BZ #16364]
> 	* sysdeps/unix/sysv/linux/sleep.c (__sleep): Simplify code to use
> 	nanosleep without requiring to handle spurious wakeups.
> 	* posix/tst-nanosleep-signal.c: New file.
> 	* posix/Makefile (test): Add tst-nanosleep-signal.

Thanks, but see below for details.

> +  int ret;
> +  pid_t f = fork ();
> +  if (f == 0)
> +    {
> +      // child
> +      struct timespec tv = { .tv_sec = 0, .tv_nsec = 250000000UL };
> +      nanosleep (&tv, &tv);
> +      exit (0);


This needs to be “_exit (0);”, to avoid running termination handlers twice.

> +    }
> +  else
> +    {
> +      // parent
> +      struct timespec tv = { .tv_sec = 0, .tv_nsec = 500000000UL };
> +      ret = nanosleep (&tv, &tv);
> +    }
> +  return ret;
> +}

The parent needs to check for fork errors.

> +
> +void
> +check_nanosleep_sigdfl(void)

Space before paren.


> +{
> +  /* For SIG_DFL nanosleep should not be interrupted by SIGCHLD.  */
> +  signal(SIGCHLD, SIG_DFL);

Space before paren.

> +  if (check_nanosleep_base ())
> +    {
> +      puts ("error: nanosleep was interrupted with SIG_DFL");
> +      errors = 1;
> +    }
> +}

This should check that errno is EINTR.

> +
> +void
> +check_nanosleep_dummy(void)

Space before paren.

> +{
> +  /* With a dummy handler nanosleep should be interrupted.  */
> +  signal(SIGCHLD, dummy);

Space before paren.

> +  int ret = check_nanosleep_base ();
> +  if (ret == 0)
> +    {
> +      puts ("error: nanosleep was not interrupted with dummy handler");
> +      errors = 1;
> +    }
> +}

Missing EINTR check.

> +
> +void
> +check_nanosleep_sigign(void)

Space before paren.

> +{
> +  /* For SIG_IGN nanosleep should not be interrupted by SIGCHLD.  */
> +  signal(SIGCHLD, SIG_IGN);

Space before paren.

The test is racy in two ways: the child could exit before nanosleep in
the parent starts, or the child exit could be delayed after nanosleep in
the parent ends.  I'm not sure if there is way to make this more reliable.

The larger question is whether the EINTR check is sufficient, or if a
time-based check is needed as well.  That is, if the kernel bug
consistent of silent early termination of nanosleep.

Florian