This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Remove signal handling for nanosleep (bug 16364)
- From: Florian Weimer <fweimer at redhat dot com>
- To: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- Cc: libc-alpha at sourceware dot org
- Date: Mon, 9 Nov 2015 15:28:40 +0100
- Subject: Re: [PATCH] Remove signal handling for nanosleep (bug 16364)
- Authentication-results: sourceware.org; auth=none
- References: <1447023171-31542-1-git-send-email-adhemerval dot zanella at linaro dot com> <56405109 dot 9070404 at redhat dot com> <56408F14 dot 1040600 at linaro dot org> <56409561 dot 7050707 at redhat dot com> <5640A89D dot 80804 at linaro dot org>
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