diff options
author | Dean Deng <deandeng@google.com> | 2020-10-16 14:30:19 -0700 |
---|---|---|
committer | gVisor bot <gvisor-bot@google.com> | 2020-10-16 14:32:49 -0700 |
commit | 4ddb58f6efb3b0bfcce40f8d126973701db89c4b (patch) | |
tree | 4d9f4f2586b8c49a49bc9ba660c0d2efee188ea3 | |
parent | dffa4c66909f034429e933f204cd49655c3e224f (diff) |
Use POSIX interval timers in flock test.
ualarm(2) is obsolete. Move IntervalTimer into a test util, where it can be
used by flock tests.
These tests were flaky with TSAN, probably because it slowed the tests down
enough that the alarm was expiring before flock() was called. Use an interval
timer so that even if we miss the first alarm (or more), flock() is still
guaranteed to be interrupted.
PiperOrigin-RevId: 337578751
-rw-r--r-- | test/syscalls/linux/BUILD | 1 | ||||
-rw-r--r-- | test/syscalls/linux/flock.cc | 46 | ||||
-rw-r--r-- | test/syscalls/linux/timers.cc | 94 | ||||
-rw-r--r-- | test/util/save_util_linux.cc | 2 | ||||
-rw-r--r-- | test/util/timer_util.cc | 18 | ||||
-rw-r--r-- | test/util/timer_util.h | 94 |
6 files changed, 151 insertions, 104 deletions
diff --git a/test/syscalls/linux/BUILD b/test/syscalls/linux/BUILD index 36b7f1b97..572f39a5d 100644 --- a/test/syscalls/linux/BUILD +++ b/test/syscalls/linux/BUILD @@ -3624,6 +3624,7 @@ cc_binary( "//test/util:signal_util", "//test/util:test_util", "//test/util:thread_util", + "//test/util:timer_util", ], ) diff --git a/test/syscalls/linux/flock.cc b/test/syscalls/linux/flock.cc index 549141cbb..b286e84fe 100644 --- a/test/syscalls/linux/flock.cc +++ b/test/syscalls/linux/flock.cc @@ -216,14 +216,29 @@ TEST_F(FlockTest, TestSharedLockFailExclusiveHolderBlocking_NoRandomSave) { const FileDescriptor fd = ASSERT_NO_ERRNO_AND_VALUE(Open(test_file_name_, O_RDWR)); - // Register a signal handler for SIGALRM and set an alarm that will go off - // while blocking in the subsequent flock() call. This will interrupt flock() - // and cause it to return EINTR. + // Make sure that a blocking flock() call will return EINTR when interrupted + // by a signal. Create a timer that will go off while blocking on flock(), and + // register the corresponding signal handler. + auto timer = ASSERT_NO_ERRNO_AND_VALUE( + TimerCreate(CLOCK_MONOTONIC, sigevent_t{ + .sigev_signo = SIGALRM, + .sigev_notify = SIGEV_SIGNAL, + })); + struct sigaction act = {}; act.sa_handler = trivial_handler; ASSERT_THAT(sigaction(SIGALRM, &act, NULL), SyscallSucceeds()); - ASSERT_THAT(ualarm(10000, 0), SyscallSucceeds()); + + // Now that the signal handler is registered, set the timer. Set an interval + // so that it's ok if the timer goes off before we call flock. + ASSERT_NO_ERRNO( + timer.Set(0, itimerspec{ + .it_interval = absl::ToTimespec(absl::Milliseconds(10)), + .it_value = absl::ToTimespec(absl::Milliseconds(10)), + })); + ASSERT_THAT(flock(fd.get(), LOCK_SH), SyscallFailsWithErrno(EINTR)); + timer.reset(); // Unlock ASSERT_THAT(flock(test_file_fd_.get(), LOCK_UN), SyscallSucceedsWithValue(0)); @@ -258,14 +273,29 @@ TEST_F(FlockTest, TestExclusiveLockFailExclusiveHolderBlocking_NoRandomSave) { const FileDescriptor fd = ASSERT_NO_ERRNO_AND_VALUE(Open(test_file_name_, O_RDWR)); - // Register a signal handler for SIGALRM and set an alarm that will go off - // while blocking in the subsequent flock() call. This will interrupt flock() - // and cause it to return EINTR. + // Make sure that a blocking flock() call will return EINTR when interrupted + // by a signal. Create a timer that will go off while blocking on flock(), and + // register the corresponding signal handler. + auto timer = ASSERT_NO_ERRNO_AND_VALUE( + TimerCreate(CLOCK_MONOTONIC, sigevent_t{ + .sigev_signo = SIGALRM, + .sigev_notify = SIGEV_SIGNAL, + })); + struct sigaction act = {}; act.sa_handler = trivial_handler; ASSERT_THAT(sigaction(SIGALRM, &act, NULL), SyscallSucceeds()); - ASSERT_THAT(ualarm(10000, 0), SyscallSucceeds()); + + // Now that the signal handler is registered, set the timer. Set an interval + // so that it's ok if the timer goes off before we call flock. + ASSERT_NO_ERRNO( + timer.Set(0, itimerspec{ + .it_interval = absl::ToTimespec(absl::Milliseconds(10)), + .it_value = absl::ToTimespec(absl::Milliseconds(10)), + })); + ASSERT_THAT(flock(fd.get(), LOCK_EX), SyscallFailsWithErrno(EINTR)); + timer.reset(); // Unlock ASSERT_THAT(flock(test_file_fd_.get(), LOCK_UN), SyscallSucceedsWithValue(0)); diff --git a/test/syscalls/linux/timers.cc b/test/syscalls/linux/timers.cc index 4b3c44527..cac94d9e1 100644 --- a/test/syscalls/linux/timers.cc +++ b/test/syscalls/linux/timers.cc @@ -33,6 +33,7 @@ #include "test/util/signal_util.h" #include "test/util/test_util.h" #include "test/util/thread_util.h" +#include "test/util/timer_util.h" ABSL_FLAG(bool, timers_test_sleep, false, "If true, sleep forever instead of running tests."); @@ -215,99 +216,6 @@ TEST(TimerTest, ProcessKilledOnCPUHardLimit) { EXPECT_GE(cpu, kHardLimit); } -// RAII type for a kernel "POSIX" interval timer. (The kernel provides system -// calls such as timer_create that behave very similarly, but not identically, -// to those described by timer_create(2); in particular, the kernel does not -// implement SIGEV_THREAD. glibc builds POSIX-compliant interval timers based on -// these kernel interval timers.) -// -// Compare implementation to FileDescriptor. -class IntervalTimer { - public: - IntervalTimer() = default; - - explicit IntervalTimer(int id) { set_id(id); } - - IntervalTimer(IntervalTimer&& orig) : id_(orig.release()) {} - - IntervalTimer& operator=(IntervalTimer&& orig) { - if (this == &orig) return *this; - reset(orig.release()); - return *this; - } - - IntervalTimer(const IntervalTimer& other) = delete; - IntervalTimer& operator=(const IntervalTimer& other) = delete; - - ~IntervalTimer() { reset(); } - - int get() const { return id_; } - - int release() { - int const id = id_; - id_ = -1; - return id; - } - - void reset() { reset(-1); } - - void reset(int id) { - if (id_ >= 0) { - TEST_PCHECK(syscall(SYS_timer_delete, id_) == 0); - MaybeSave(); - } - set_id(id); - } - - PosixErrorOr<struct itimerspec> Set( - int flags, const struct itimerspec& new_value) const { - struct itimerspec old_value = {}; - if (syscall(SYS_timer_settime, id_, flags, &new_value, &old_value) < 0) { - return PosixError(errno, "timer_settime"); - } - MaybeSave(); - return old_value; - } - - PosixErrorOr<struct itimerspec> Get() const { - struct itimerspec curr_value = {}; - if (syscall(SYS_timer_gettime, id_, &curr_value) < 0) { - return PosixError(errno, "timer_gettime"); - } - MaybeSave(); - return curr_value; - } - - PosixErrorOr<int> Overruns() const { - int rv = syscall(SYS_timer_getoverrun, id_); - if (rv < 0) { - return PosixError(errno, "timer_getoverrun"); - } - MaybeSave(); - return rv; - } - - private: - void set_id(int id) { id_ = std::max(id, -1); } - - // Kernel timer_t is int; glibc timer_t is void*. - int id_ = -1; -}; - -PosixErrorOr<IntervalTimer> TimerCreate(clockid_t clockid, - const struct sigevent& sev) { - int timerid; - int ret = syscall(SYS_timer_create, clockid, &sev, &timerid); - if (ret < 0) { - return PosixError(errno, "timer_create"); - } - if (ret > 0) { - return PosixError(EINVAL, "timer_create should never return positive"); - } - MaybeSave(); - return IntervalTimer(timerid); -} - // See timerfd.cc:TimerSlack() for rationale. constexpr absl::Duration kTimerSlack = absl::Milliseconds(500); diff --git a/test/util/save_util_linux.cc b/test/util/save_util_linux.cc index d0aea8e6a..fbac94912 100644 --- a/test/util/save_util_linux.cc +++ b/test/util/save_util_linux.cc @@ -46,4 +46,4 @@ void MaybeSave() { } // namespace testing } // namespace gvisor -#endif +#endif // __linux__ diff --git a/test/util/timer_util.cc b/test/util/timer_util.cc index 43a26b0d3..75cfc4f40 100644 --- a/test/util/timer_util.cc +++ b/test/util/timer_util.cc @@ -23,5 +23,23 @@ absl::Time Now(clockid_t id) { return absl::TimeFromTimespec(now); } +#ifdef __linux__ + +PosixErrorOr<IntervalTimer> TimerCreate(clockid_t clockid, + const struct sigevent& sev) { + int timerid; + int ret = syscall(SYS_timer_create, clockid, &sev, &timerid); + if (ret < 0) { + return PosixError(errno, "timer_create"); + } + if (ret > 0) { + return PosixError(EINVAL, "timer_create should never return positive"); + } + MaybeSave(); + return IntervalTimer(timerid); +} + +#endif // __linux__ + } // namespace testing } // namespace gvisor diff --git a/test/util/timer_util.h b/test/util/timer_util.h index 31aea4fc6..926e6632f 100644 --- a/test/util/timer_util.h +++ b/test/util/timer_util.h @@ -16,6 +16,9 @@ #define GVISOR_TEST_UTIL_TIMER_UTIL_H_ #include <errno.h> +#ifdef __linux__ +#include <sys/syscall.h> +#endif #include <sys/time.h> #include <functional> @@ -30,6 +33,9 @@ namespace gvisor { namespace testing { +// Returns the current time. +absl::Time Now(clockid_t id); + // MonotonicTimer is a simple timer that uses a monotonic clock. class MonotonicTimer { public: @@ -65,8 +71,92 @@ inline PosixErrorOr<Cleanup> ScopedItimer(int which, })); } -// Returns the current time. -absl::Time Now(clockid_t id); +#ifdef __linux__ + +// RAII type for a kernel "POSIX" interval timer. (The kernel provides system +// calls such as timer_create that behave very similarly, but not identically, +// to those described by timer_create(2); in particular, the kernel does not +// implement SIGEV_THREAD. glibc builds POSIX-compliant interval timers based on +// these kernel interval timers.) +// +// Compare implementation to FileDescriptor. +class IntervalTimer { + public: + IntervalTimer() = default; + + explicit IntervalTimer(int id) { set_id(id); } + + IntervalTimer(IntervalTimer&& orig) : id_(orig.release()) {} + + IntervalTimer& operator=(IntervalTimer&& orig) { + if (this == &orig) return *this; + reset(orig.release()); + return *this; + } + + IntervalTimer(const IntervalTimer& other) = delete; + IntervalTimer& operator=(const IntervalTimer& other) = delete; + + ~IntervalTimer() { reset(); } + + int get() const { return id_; } + + int release() { + int const id = id_; + id_ = -1; + return id; + } + + void reset() { reset(-1); } + + void reset(int id) { + if (id_ >= 0) { + TEST_PCHECK(syscall(SYS_timer_delete, id_) == 0); + MaybeSave(); + } + set_id(id); + } + + PosixErrorOr<struct itimerspec> Set( + int flags, const struct itimerspec& new_value) const { + struct itimerspec old_value = {}; + if (syscall(SYS_timer_settime, id_, flags, &new_value, &old_value) < 0) { + return PosixError(errno, "timer_settime"); + } + MaybeSave(); + return old_value; + } + + PosixErrorOr<struct itimerspec> Get() const { + struct itimerspec curr_value = {}; + if (syscall(SYS_timer_gettime, id_, &curr_value) < 0) { + return PosixError(errno, "timer_gettime"); + } + MaybeSave(); + return curr_value; + } + + PosixErrorOr<int> Overruns() const { + int rv = syscall(SYS_timer_getoverrun, id_); + if (rv < 0) { + return PosixError(errno, "timer_getoverrun"); + } + MaybeSave(); + return rv; + } + + private: + void set_id(int id) { id_ = std::max(id, -1); } + + // Kernel timer_t is int; glibc timer_t is void*. + int id_ = -1; +}; + +// A wrapper around timer_create(2). +PosixErrorOr<IntervalTimer> TimerCreate(clockid_t clockid, + const struct sigevent& sev); + +#endif // __linux__ } // namespace testing } // namespace gvisor |