From 992e42977bb39c34360e5224a85115a1a9f834dc Mon Sep 17 00:00:00 2001 From: Alex Deymo Date: Fri, 11 Dec 2015 19:42:09 -0800 Subject: Fix Process unittests. The unused file descriptor tests were flaky because they had a 10ms sleep were the child process was supposed to run. While 10ms is typically enough for that, our builders run at a high load at 10ms could not be enough. This patch uses a different approach to test the same behavior by checking if the file descriptors are available from the same child process. It also fixes a fd leak by used a ScopedPipe. Bug: None TEST=FEATURES=test emerge-link libbrillo Change-Id: I53d529c5dbebbfa1af4cc858d76a200d3344d3ff --- brillo/message_loops/message_loop_unittest.cc | 62 +-------------------------- brillo/process_unittest.cc | 44 ++++++------------- brillo/unittest_utils.cc | 53 +++++++++++++++++++++++ brillo/unittest_utils.h | 43 +++++++++++++++++++ libbrillo.gypi | 1 + 5 files changed, 112 insertions(+), 91 deletions(-) create mode 100644 brillo/unittest_utils.cc create mode 100644 brillo/unittest_utils.h diff --git a/brillo/message_loops/message_loop_unittest.cc b/brillo/message_loops/message_loop_unittest.cc index 20fb6be..7cc2a30 100644 --- a/brillo/message_loops/message_loop_unittest.cc +++ b/brillo/message_loops/message_loop_unittest.cc @@ -9,11 +9,6 @@ // implementation-specific tests see the particular implementation unittests in // the *_unittest.cc files. -#include -#include -#include -#include - #include #include @@ -23,6 +18,7 @@ #include #include +#include #include #include #include @@ -30,62 +26,6 @@ using base::Bind; using base::TimeDelta; -namespace { -// Helper class to create and close a unidirectional pipe. Used to provide valid -// file descriptors when testing watching for a file descriptor. -class ScopedPipe { - public: - // The internal pipe size. - static const int kPipeSize; - - ScopedPipe() { - int fds[2]; - if (pipe(fds) != 0) { - PLOG(FATAL) << "Creating a pipe()"; - } - reader = fds[0]; - writer = fds[1]; - EXPECT_EQ(kPipeSize, fcntl(writer, F_SETPIPE_SZ, kPipeSize)); - } - ~ScopedPipe() { - if (reader != -1) - close(reader); - if (writer != -1) - close(writer); - } - - // The reader and writer end of the pipe. - int reader{-1}; - int writer{-1}; -}; - -const int ScopedPipe::kPipeSize = 4096; - -class ScopedSocketPair { - public: - ScopedSocketPair() { - int fds[2]; - if (socketpair(PF_LOCAL, SOCK_STREAM, 0, fds) != 0) { - PLOG(FATAL) << "Creating a socketpair()"; - } - left = fds[0]; - right = fds[1]; - } - ~ScopedSocketPair() { - if (left != -1) - close(left); - if (right != -1) - close(right); - } - - // The left and right sockets are bi-directional connected and - // indistinguishable file descriptor. We named them left/right for easier - // reading. - int left{-1}; - int right{-1}; -}; -} // namespace - namespace brillo { using TaskId = MessageLoop::TaskId; diff --git a/brillo/process_unittest.cc b/brillo/process_unittest.cc index 16c1ecc..de6cd72 100644 --- a/brillo/process_unittest.cc +++ b/brillo/process_unittest.cc @@ -12,6 +12,7 @@ #include #include "brillo/process_mock.h" +#include "brillo/unittest_utils.h" #include "brillo/test_helpers.h" using base::FilePath; @@ -29,6 +30,7 @@ static const char kBinCp[] = SYSTEM_PREFIX "/bin/cp"; static const char kBinEcho[] = SYSTEM_PREFIX "/bin/echo"; static const char kBinFalse[] = SYSTEM_PREFIX "/bin/false"; static const char kBinSleep[] = SYSTEM_PREFIX "/bin/sleep"; +static const char kBinStat[] = SYSTEM_PREFIX "/usr/bin/stat"; static const char kBinTrue[] = SYSTEM_PREFIX "/bin/true"; namespace brillo { @@ -94,7 +96,6 @@ class ProcessTest : public ::testing::Test { protected: void CheckStderrCaptured(); FilePath GetFdPath(int fd); - bool FileDescriptorExists(int pid, int fd); ProcessImpl process_; std::vector args_; @@ -168,11 +169,6 @@ FilePath ProcessTest::GetFdPath(int fd) { return FilePath(base::StringPrintf("/proc/self/fd/%d", fd)); } -bool ProcessTest::FileDescriptorExists(int pid, int fd) { - return base::PathExists( - FilePath(base::StringPrintf("/proc/%d/fd/%d", pid, fd))); -} - TEST_F(ProcessTest, RedirectStderrUsingPipe) { std::string contents; process_.RedirectOutput(""); @@ -341,35 +337,23 @@ TEST_F(ProcessTest, PreExecCallback) { } TEST_F(ProcessTest, LeakUnusedFileDescriptors) { - int fds[2]; - EXPECT_EQ(0, pipe(fds)); - process_.AddArg(kBinSleep); - process_.AddArg("10000"); + ScopedPipe pipe; + process_.AddArg(kBinStat); + process_.AddArg(GetFdPath(pipe.reader).value()); + process_.AddArg(GetFdPath(pipe.writer).value()); process_.SetCloseUnusedFileDescriptors(false); - ASSERT_TRUE(process_.Start()); - // Give child process a bit time to come up. - usleep(10 * 1000); - // Verify file descriptors are leaking to the child process. - EXPECT_TRUE(FileDescriptorExists(process_.pid(), fds[0])); - EXPECT_TRUE(FileDescriptorExists(process_.pid(), fds[1])); - - EXPECT_TRUE(process_.Kill(SIGTERM, 1)); + EXPECT_EQ(0, process_.Run()); } TEST_F(ProcessTest, CloseUnusedFileDescriptors) { - int fds[2]; - EXPECT_EQ(0, pipe(fds)); - process_.AddArg(kBinSleep); - process_.AddArg("10000"); + ScopedPipe pipe; + process_.AddArg(kBinStat); + process_.AddArg(GetFdPath(pipe.reader).value()); + process_.AddArg(GetFdPath(pipe.writer).value()); process_.SetCloseUnusedFileDescriptors(true); - ASSERT_TRUE(process_.Start()); - // Give child process a bit time to come up. - usleep(10 * 1000); - // Verify file descriptors does not get leak to the child process. - EXPECT_FALSE(FileDescriptorExists(process_.pid(), fds[0])); - EXPECT_FALSE(FileDescriptorExists(process_.pid(), fds[1])); - - EXPECT_TRUE(process_.Kill(SIGTERM, 1)); + // Stat should fail when running on these file descriptor because the files + // should not be there. + EXPECT_EQ(1, process_.Run()); } } // namespace brillo diff --git a/brillo/unittest_utils.cc b/brillo/unittest_utils.cc new file mode 100644 index 0000000..7cace5d --- /dev/null +++ b/brillo/unittest_utils.cc @@ -0,0 +1,53 @@ +// Copyright 2014 The Chromium OS Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include + +#include +#include +#include +#include + +#include +#include + +namespace brillo { + +const int ScopedPipe::kPipeSize = 4096; + +ScopedPipe::ScopedPipe() { + int fds[2]; + if (pipe(fds) != 0) { + PLOG(FATAL) << "Creating a pipe()"; + } + reader = fds[0]; + writer = fds[1]; + EXPECT_EQ(kPipeSize, fcntl(writer, F_SETPIPE_SZ, kPipeSize)); +} + +ScopedPipe::~ScopedPipe() { + if (reader != -1) + close(reader); + if (writer != -1) + close(writer); +} + + +ScopedSocketPair::ScopedSocketPair() { + int fds[2]; + if (socketpair(PF_LOCAL, SOCK_STREAM, 0, fds) != 0) { + PLOG(FATAL) << "Creating a socketpair()"; + } + left = fds[0]; + right = fds[1]; +} + +ScopedSocketPair::~ScopedSocketPair() { + if (left != -1) + close(left); + if (right != -1) + close(right); +} + +} // namespace brillo diff --git a/brillo/unittest_utils.h b/brillo/unittest_utils.h new file mode 100644 index 0000000..a7f92db --- /dev/null +++ b/brillo/unittest_utils.h @@ -0,0 +1,43 @@ +// Copyright 2015 The Chromium OS Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Internal implementation of brillo::Any class. + +#ifndef LIBBRILLO_BRILLO_UNITTEST_UTILS_H_ +#define LIBBRILLO_BRILLO_UNITTEST_UTILS_H_ + +namespace brillo { + +// Helper class to create and close a unidirectional pipe. The file descriptors +// will be closed on destruction, unless set to -1. +class ScopedPipe { + public: + // The internal pipe size. + static const int kPipeSize; + + ScopedPipe(); + ~ScopedPipe(); + + // The reader and writer end of the pipe. + int reader{-1}; + int writer{-1}; +}; + +// Helper class to create and close a bi-directional pair of sockets. The +// sockets will be closed on destruction, unless set to -1. +class ScopedSocketPair { + public: + ScopedSocketPair(); + ~ScopedSocketPair(); + + // The left and right sockets are bi-directional connected and + // indistinguishable file descriptor. We named them left/right for easier + // reading. + int left{-1}; + int right{-1}; +}; + +} // namespace brillo + +#endif // LIBBRILLO_BRILLO_UNITTEST_UTILS_H_ diff --git a/libbrillo.gypi b/libbrillo.gypi index d0b3504..6dec2e4 100644 --- a/libbrillo.gypi +++ b/libbrillo.gypi @@ -353,6 +353,7 @@ 'brillo/streams/stream_unittest.cc', 'brillo/streams/stream_utils_unittest.cc', 'brillo/strings/string_utils_unittest.cc', + 'brillo/unittest_utils.cc', 'brillo/url_utils_unittest.cc', 'brillo/variant_dictionary_unittest.cc', 'testrunner.cc', -- cgit v1.2.3