update_engine: Fix leaking unit tests
Some of the unit tests have been leaking temp files because they don't
properly unlink them. In this CL, we did some rearrangement of the
ScopedTempFile class and moved it into the utils.h (instead of testing
only location) so it can be used everywhere and more efficiently. Also
added functionality to open an file descriptor too so users don't have
to keep a different object for the file descriptor.
BUG=b:162766400
TEST=cros_workon_make --board reef --test; Then looked at the
/build/reef/tmp directory and no files were leaked.
Change-Id: Id64a2923d30f27628120497fdefe16bf65fa3fb0
Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/2500772
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Jae Hoon Kim <kimjae@chromium.org>
Commit-Queue: Amin Hassani <ahassani@chromium.org>
diff --git a/common/hash_calculator_unittest.cc b/common/hash_calculator_unittest.cc
index e8f73d5..fe7d543 100644
--- a/common/hash_calculator_unittest.cc
+++ b/common/hash_calculator_unittest.cc
@@ -104,7 +104,7 @@
}
TEST_F(HashCalculatorTest, UpdateFileSimpleTest) {
- test_utils::ScopedTempFile data_file("data.XXXXXX");
+ ScopedTempFile data_file("data.XXXXXX");
ASSERT_TRUE(test_utils::WriteFileString(data_file.path(), "hi"));
for (const int length : {-1, 2, 10}) {
@@ -126,7 +126,7 @@
}
TEST_F(HashCalculatorTest, RawHashOfFileSimpleTest) {
- test_utils::ScopedTempFile data_file("data.XXXXXX");
+ ScopedTempFile data_file("data.XXXXXX");
ASSERT_TRUE(test_utils::WriteFileString(data_file.path(), "hi"));
for (const int length : {-1, 2, 10}) {
diff --git a/common/http_fetcher_unittest.cc b/common/http_fetcher_unittest.cc
index 1ead681..99ea99b 100644
--- a/common/http_fetcher_unittest.cc
+++ b/common/http_fetcher_unittest.cc
@@ -369,7 +369,7 @@
HttpServer* CreateServer() override { return new NullHttpServer; }
private:
- test_utils::ScopedTempFile temp_file_{"ue_file_fetcher.XXXXXX"};
+ ScopedTempFile temp_file_{"ue_file_fetcher.XXXXXX"};
};
class MultiRangeHttpFetcherOverFileFetcherTest : public FileFetcherTest {
diff --git a/common/test_utils.h b/common/test_utils.h
index 63ea749..bb5a678 100644
--- a/common/test_utils.h
+++ b/common/test_utils.h
@@ -138,22 +138,6 @@
DISALLOW_COPY_AND_ASSIGN(ScopedLoopbackDeviceBinder);
};
-class ScopedTempFile {
- public:
- ScopedTempFile() : ScopedTempFile("update_engine_test_temp_file.XXXXXX") {}
-
- explicit ScopedTempFile(const std::string& pattern) {
- EXPECT_TRUE(utils::MakeTempFile(pattern, &path_, nullptr));
- unlinker_.reset(new ScopedPathUnlinker(path_));
- }
-
- const std::string& path() const { return path_; }
-
- private:
- std::string path_;
- std::unique_ptr<ScopedPathUnlinker> unlinker_;
-};
-
class ScopedLoopMounter {
public:
explicit ScopedLoopMounter(const std::string& file_path,
diff --git a/common/utils.h b/common/utils.h
index f364bfd..05a92be 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -370,6 +370,42 @@
DISALLOW_COPY_AND_ASSIGN(ScopedPathUnlinker);
};
+class ScopedTempFile {
+ public:
+ ScopedTempFile() : ScopedTempFile("update_engine_temp.XXXXXX") {}
+
+ // If |open_fd| is true, a writable file descriptor will be opened for this
+ // file.
+ explicit ScopedTempFile(const std::string& pattern, bool open_fd = false) {
+ CHECK(utils::MakeTempFile(pattern, &path_, open_fd ? &fd_ : nullptr));
+ unlinker_.reset(new ScopedPathUnlinker(path_));
+ if (open_fd) {
+ CHECK_GE(fd_, 0);
+ fd_closer_.reset(new ScopedFdCloser(&fd_));
+ }
+ }
+ virtual ~ScopedTempFile() = default;
+
+ const std::string& path() const { return path_; }
+ int fd() const {
+ CHECK(fd_closer_);
+ return fd_;
+ }
+ void CloseFd() {
+ CHECK(fd_closer_);
+ fd_closer_.reset();
+ }
+
+ private:
+ std::string path_;
+ std::unique_ptr<ScopedPathUnlinker> unlinker_;
+
+ int fd_{-1};
+ std::unique_ptr<ScopedFdCloser> fd_closer_;
+
+ DISALLOW_COPY_AND_ASSIGN(ScopedTempFile);
+};
+
// A little object to call ActionComplete on the ActionProcessor when
// it's destructed.
class ScopedActionCompleter {
diff --git a/common/utils_unittest.cc b/common/utils_unittest.cc
index bc5c3b0..20c6b84 100644
--- a/common/utils_unittest.cc
+++ b/common/utils_unittest.cc
@@ -46,7 +46,7 @@
}
TEST(UtilsTest, WriteFileReadFile) {
- test_utils::ScopedTempFile file;
+ ScopedTempFile file;
EXPECT_TRUE(utils::WriteFile(file.path().c_str(), "hello", 5));
brillo::Blob readback;
@@ -60,7 +60,7 @@
}
TEST(UtilsTest, ReadFileChunk) {
- test_utils::ScopedTempFile file;
+ ScopedTempFile file;
brillo::Blob data;
const size_t kSize = 1024 * 1024;
for (size_t i = 0; i < kSize; i++) {
@@ -149,7 +149,7 @@
namespace {
void GetFileFormatTester(const string& expected,
const vector<uint8_t>& contents) {
- test_utils::ScopedTempFile file;
+ ScopedTempFile file;
ASSERT_TRUE(utils::WriteFile(file.path().c_str(),
reinterpret_cast<const char*>(contents.data()),
contents.size()));
@@ -378,7 +378,7 @@
}
TEST(UtilsTest, RunAsRootUnmountFilesystemBusyFailureTest) {
- test_utils::ScopedTempFile tmp_image("img.XXXXXX");
+ ScopedTempFile tmp_image("img.XXXXXX");
EXPECT_TRUE(base::CopyFile(
test_utils::GetBuildArtifactsPath().Append("gen/disk_ext2_4k.img"),
@@ -418,7 +418,7 @@
EXPECT_TRUE(mnt_dir.CreateUniqueTempDir());
EXPECT_FALSE(utils::IsMountpoint(mnt_dir.GetPath().value()));
- test_utils::ScopedTempFile file;
+ ScopedTempFile file;
EXPECT_FALSE(utils::IsMountpoint(file.path()));
}
@@ -460,7 +460,7 @@
}
TEST(UtilsTest, GetFilePathTest) {
- test_utils::ScopedTempFile file;
+ ScopedTempFile file;
int fd = HANDLE_EINTR(open(file.path().c_str(), O_RDONLY));
EXPECT_GE(fd, 0);
EXPECT_EQ(file.path(), utils::GetFilePath(fd));