Re: [PATCH 2/3] Refactor nanosleep in terms of clock_nanosleep

["Dmitry V. Levin" <ldv at altlinux dot org>]

On Fri, Nov 29, 2019 at 06:21:53PM +0300, Dmitry V. Levin wrote:
> On Fri, Nov 29, 2019 at 08:27:06AM -0300, Adhemerval Zanella wrote:
> > On 28/11/2019 20:44, Dmitry V. Levin wrote:
> > > On Wed, Nov 06, 2019 at 09:52:44AM -0300, Adhemerval Zanella wrote:
> > >> The generic version is straightforward.  For Hurd, its nanosleep
> > >> implementation is moved to clock_nanosleep with adjustments from
> > >> generic unix implementation.
> > >>
> > >> The generic clock_nanosleep unix version is also removed since
> > >> it calls nanosleep.
> > >>
> > >> Checked on x86_64-linux-gnu and powerpc64le-linux-gnu.
> > >> ---
> > >>  include/time.h                            |  3 +
> > >>  posix/nanosleep.c                         | 13 ++--
> > >>  sysdeps/{unix => mach}/clock_nanosleep.c  | 79 +++++++++++++++++------
> > >>  sysdeps/mach/nanosleep.c                  | 79 -----------------------
> > >>  sysdeps/unix/sysv/linux/clock_nanosleep.c |  2 +-
> > >>  sysdeps/unix/sysv/linux/nanosleep.c       | 31 ---------
> > >>  time/clock_nanosleep.c                    |  2 +-
> > >>  7 files changed, 71 insertions(+), 138 deletions(-)
> > >>  rename sysdeps/{unix => mach}/clock_nanosleep.c (59%)
> > >>  delete mode 100644 sysdeps/mach/nanosleep.c
> > >>  delete mode 100644 sysdeps/unix/sysv/linux/nanosleep.c
> > >>
> > >> diff --git a/include/time.h b/include/time.h
> > >> index 8ac58e891b..b3e635395d 100644
> > >> --- a/include/time.h
> > >> +++ b/include/time.h
> > >> @@ -25,6 +25,9 @@ libc_hidden_proto (__clock_gettime)
> > >>  extern __typeof (clock_settime) __clock_settime;
> > >>  libc_hidden_proto (__clock_settime)
> > >>  
> > >> +extern __typeof (clock_nanosleep) __clock_nanosleep;
> > >> +libc_hidden_proto (__clock_nanosleep);
> > >> +
> > >>  #ifdef __linux__
> > >>  extern __typeof (clock_adjtime) __clock_adjtime;
> > >>  libc_hidden_proto (__clock_adjtime);
> > >> diff --git a/posix/nanosleep.c b/posix/nanosleep.c
> > >> index d8564c7119..ed41c8cce7 100644
> > >> --- a/posix/nanosleep.c
> > >> +++ b/posix/nanosleep.c
> > >> @@ -24,10 +24,13 @@ int
> > >>  __nanosleep (const struct timespec *requested_time,
> > >>  	     struct timespec *remaining)
> > >>  {
> > >> -  __set_errno (ENOSYS);
> > >> -  return -1;
> > >> +  int ret = __clock_nanosleep (CLOCK_REALTIME, 0, requested_time, remaining);
> > > 
> > > Shouldn't it set flags to TIMER_ABSTIME?
> > > 
> > > In the current form it causes a regression when the syscall is interrupted
> > > by signal - "remaining" is updated by the kernel but __clock_nanosleep
> > > leaves it unchanged.
> > 
> > I am not seeing this regression:
> 
> The regression has been detected by the strace test suite on Rawhide,
> the log is at
> https://kojipkgs.fedoraproject.org/work/tasks/1866/39391866/build.log
> (look for "FAIL: restart_syscall" there), the test is
> https://github.com/strace/strace/blob/master/tests/restart_syscall.c

Here is a stripped down example:
$ cat nanorem.c
#include <err.h>
#include <signal.h>
#include <stdint.h>
#include <stdio.h>
#include <sys/time.h>
#include <time.h>

int
main(void)
{
	if (signal(SIGALRM, SIG_IGN))
		err(1, "signal");

	const sigset_t set = {};
	if (sigprocmask(SIG_SETMASK, &set, NULL))
		err(1, "sigprocmask");

	const struct itimerval itv = { .it_value.tv_usec = 111111 };
	const struct timespec req = { .tv_nsec = 222222222 };
	const struct timespec bad = { 0xbadc0ded, 0xbadc0ded };

	struct timespec rem = bad;
	if (setitimer(ITIMER_REAL, &itv, NULL))
		err(1, "setitimer");
	if (nanosleep(&req, &rem))
		err(1, "nanosleep");
	printf("nanosleep: tv_sec=%lld, tv_nsec=%llu\n",
	       (long long) rem.tv_sec, (unsigned long long) rem.tv_nsec);

	rem = bad;
	if (setitimer(ITIMER_REAL, &itv, NULL))
		err(1, "setitimer");
	if (clock_nanosleep(CLOCK_REALTIME, 0, &req, &rem))
		err(1, "clock_nanosleep");
	printf("clock_nanosleep: tv_sec=%lld, tv_nsec=%llu\n",
	       (long long) rem.tv_sec, (unsigned long long) rem.tv_nsec);

	return 0;
}
$ gcc -Wall -O2 nanorem.c -o nanorem
$ strace -qq -enone -esignal=none ./nanorem
nanosleep: tv_sec=0, tv_nsec=111185098
clock_nanosleep: tv_sec=0, tv_nsec=111173210

Apparently, the output for nanosleep is no longer the same after the
change.


-- 
ldv