From 3cb682e3698775f0ea80cb6cfac844c6acf91540 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Tue, 6 Aug 2019 15:06:00 -0700 Subject: [PATCH] libsnapshot: Improve first test-run and test cleanup. This CL fixes a bug where libsnapshot_test failed on the first run. It also fixes bugs where it could not run if it died in the middle of a test. Previously, libsnapshot_test relied on CancelUpdate() to perform cleanup, which cannot run in certain states. Instead, manually delete dm devices and COW image files, and forcefully erase any lingering data. Bug: 136678799 Test: libsnapshot_test gtest Change-Id: I7b2399a403b387eb47184626e71dcf8674f6ab89 --- .../include/libsnapshot/snapshot.h | 2 + fs_mgr/libsnapshot/snapshot.cpp | 19 ++++++++- fs_mgr/libsnapshot/snapshot_test.cpp | 40 +++++++++++++++++-- 3 files changed, 56 insertions(+), 5 deletions(-) diff --git a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h index 062e00b67..b18a229ec 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h @@ -189,6 +189,7 @@ class SnapshotManager final { std::unique_ptr LockExclusive(); UpdateState ReadUpdateState(LockedFile* file); bool WriteUpdateState(LockedFile* file, UpdateState state); + std::string GetStateFilePath() const; // This state is persisted per-snapshot in /metadata/ota/snapshots/. struct SnapshotStatus { @@ -205,6 +206,7 @@ class SnapshotManager final { int lock_flags); bool WriteSnapshotStatus(LockedFile* file, const SnapshotStatus& status); bool ReadSnapshotStatus(LockedFile* file, SnapshotStatus* status); + std::string GetSnapshotStatusFilePath(const std::string& name); // Return the name of the device holding the "snapshot" or "snapshot-merge" // target. This may not be the final device presented via MapSnapshot(), if diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index ef561797f..dd92e5c8a 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -342,6 +342,12 @@ bool SnapshotManager::RemoveAllSnapshots(LockedFile* lock) { } UpdateState SnapshotManager::GetUpdateState(double* progress) { + // If we've never started an update, the state file won't exist. + auto state_file = GetStateFilePath(); + if (access(state_file.c_str(), F_OK) != 0 && errno == ENOENT) { + return UpdateState::None; + } + auto file = LockShared(); if (!file) { return UpdateState::None; @@ -397,9 +403,13 @@ SnapshotManager::LockedFile::~LockedFile() { } } +std::string SnapshotManager::GetStateFilePath() const { + return metadata_dir_ + "/state"s; +} + std::unique_ptr SnapshotManager::OpenStateFile(int open_flags, int lock_flags) { - auto state_file = metadata_dir_ + "/state"s; + auto state_file = GetStateFilePath(); return OpenFile(state_file, open_flags, lock_flags); } @@ -471,9 +481,14 @@ bool SnapshotManager::WriteUpdateState(LockedFile* file, UpdateState state) { return true; } +std::string SnapshotManager::GetSnapshotStatusFilePath(const std::string& name) { + auto file = metadata_dir_ + "/snapshots/"s + name; + return file; +} + auto SnapshotManager::OpenSnapshotStatusFile(const std::string& name, int open_flags, int lock_flags) -> std::unique_ptr { - auto file = metadata_dir_ + "/snapshots/"s + name; + auto file = GetSnapshotStatusFilePath(name); return OpenFile(file, open_flags, lock_flags); } diff --git a/fs_mgr/libsnapshot/snapshot_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp index 9cc9bd7d1..db15aa26a 100644 --- a/fs_mgr/libsnapshot/snapshot_test.cpp +++ b/fs_mgr/libsnapshot/snapshot_test.cpp @@ -22,13 +22,19 @@ #include #include +#include #include +#include #include +#include #include namespace android { namespace snapshot { +using android::base::unique_fd; +using android::dm::DeviceMapper; +using android::dm::DmDeviceState; using namespace std::chrono_literals; using namespace std::string_literals; @@ -48,12 +54,15 @@ std::unique_ptr sm; TestDeviceInfo* test_device = nullptr; class SnapshotTest : public ::testing::Test { + public: + SnapshotTest() : dm_(DeviceMapper::Instance()) {} + protected: void SetUp() override { test_device->set_is_running_snapshot(false); if (sm->GetUpdateState() != UpdateState::None) { - ASSERT_TRUE(sm->CancelUpdate()); + CleanupTestArtifacts(); } ASSERT_TRUE(sm->BeginUpdate()); ASSERT_TRUE(sm->EnsureImageManager()); @@ -65,13 +74,37 @@ class SnapshotTest : public ::testing::Test { void TearDown() override { lock_ = nullptr; - if (sm->GetUpdateState() != UpdateState::None) { - ASSERT_TRUE(sm->CancelUpdate()); + CleanupTestArtifacts(); + } + + void CleanupTestArtifacts() { + // Normally cancelling inside a merge is not allowed. Since these + // are tests, we don't care, destroy everything that might exist. + std::vector snapshots = {"test-snapshot"}; + for (const auto& snapshot : snapshots) { + if (dm_.GetState(snapshot) != DmDeviceState::INVALID) { + dm_.DeleteDevice(snapshot); + } + if (dm_.GetState(snapshot + "-inner") != DmDeviceState::INVALID) { + dm_.DeleteDevice(snapshot + "-inner"); + } + temp_images_.emplace_back(snapshot + "-cow"); + + auto status_file = sm->GetSnapshotStatusFilePath(snapshot); + android::base::RemoveFileIfExists(status_file); } + + // Remove all images. + temp_images_.emplace_back("test-snapshot-cow"); for (const auto& temp_image : temp_images_) { image_manager_->UnmapImageDevice(temp_image); image_manager_->DeleteBackingImage(temp_image); } + + if (sm->GetUpdateState() != UpdateState::None) { + auto state_file = sm->GetStateFilePath(); + unlink(state_file.c_str()); + } } bool AcquireLock() { @@ -87,6 +120,7 @@ class SnapshotTest : public ::testing::Test { return image_manager_->MapImageDevice(name, 10s, path); } + DeviceMapper& dm_; std::unique_ptr lock_; std::vector temp_images_; android::fiemap::IImageManager* image_manager_ = nullptr;