From 111bec1e902dabd79ec1c4e7b97b873a7c9a168f Mon Sep 17 00:00:00 2001 From: David Anderson Date: Tue, 7 Feb 2023 04:41:17 +0000 Subject: [PATCH 1/2] libsnapshot: Improve low space tests. These tests have historically been pretty flaky, and have gone through a few rounds of improvement. This is yet another: remove the parameterization, so we can test that at least one variant of the test succeeds, rather than all of them. Filesystems (especially F2FS) have a lot going on underneath the hood, and there's no guarantee that precise free space can be allocated, measured, or relied upon in the ways this test expects. And all that we really need to test is that some kind of out-of-space error can be triggered. Bug: 266645706 Test: vts_libsnapshot_test Change-Id: I9620e5b496d5020b21cc37074e87dd21fc419ed2 --- fs_mgr/libsnapshot/snapshot_test.cpp | 48 +++++++++++++++------------- fs_mgr/libsnapshot/test_helpers.cpp | 2 +- 2 files changed, 26 insertions(+), 24 deletions(-) diff --git a/fs_mgr/libsnapshot/snapshot_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp index 40143802b..13314daab 100644 --- a/fs_mgr/libsnapshot/snapshot_test.cpp +++ b/fs_mgr/libsnapshot/snapshot_test.cpp @@ -2669,44 +2669,46 @@ INSTANTIATE_TEST_SUITE_P(Snapshot, FlashAfterUpdateTest, Combine(Values(0, 1), B "Merge"s; }); -// Test behavior of ImageManager::Create on low space scenario. These tests assumes image manager -// uses /data as backup device. -class ImageManagerTest : public SnapshotTest, public WithParamInterface { +class ImageManagerTest : public SnapshotTest { protected: void SetUp() override { SKIP_IF_NON_VIRTUAL_AB(); SnapshotTest::SetUp(); - userdata_ = std::make_unique(); - ASSERT_TRUE(userdata_->Init(GetParam())); } void TearDown() override { RETURN_IF_NON_VIRTUAL_AB(); - + CleanUp(); + } + void CleanUp() { EXPECT_TRUE(!image_manager_->BackingImageExists(kImageName) || image_manager_->DeleteBackingImage(kImageName)); } + static constexpr const char* kImageName = "my_image"; - std::unique_ptr userdata_; }; -TEST_P(ImageManagerTest, CreateImageNoSpace) { - uint64_t to_allocate = userdata_->free_space() + userdata_->bsize(); - auto res = image_manager_->CreateBackingImage(kImageName, to_allocate, - IImageManager::CREATE_IMAGE_DEFAULT); - ASSERT_FALSE(res) << "Should not be able to create image with size = " << to_allocate - << " bytes because only " << userdata_->free_space() << " bytes are free"; - ASSERT_EQ(FiemapStatus::ErrorCode::NO_SPACE, res.error_code()) << res.string(); -} - -std::vector ImageManagerTestParams() { - std::vector ret; +TEST_F(ImageManagerTest, CreateImageNoSpace) { + bool at_least_one_failure = false; for (uint64_t size = 1_MiB; size <= 512_MiB; size *= 2) { - ret.push_back(size); - } - return ret; -} + auto userdata = std::make_unique(); + ASSERT_TRUE(userdata->Init(size)); -INSTANTIATE_TEST_SUITE_P(ImageManagerTest, ImageManagerTest, ValuesIn(ImageManagerTestParams())); + uint64_t to_allocate = userdata->free_space() + userdata->bsize(); + + auto res = image_manager_->CreateBackingImage(kImageName, to_allocate, + IImageManager::CREATE_IMAGE_DEFAULT); + if (!res) { + at_least_one_failure = true; + } else { + ASSERT_EQ(res.error_code(), FiemapStatus::ErrorCode::NO_SPACE) << res.string(); + } + + CleanUp(); + } + + ASSERT_TRUE(at_least_one_failure) + << "We should have failed to allocate at least one over-sized image"; +} bool Mkdir(const std::string& path) { if (mkdir(path.c_str(), 0700) && errno != EEXIST) { diff --git a/fs_mgr/libsnapshot/test_helpers.cpp b/fs_mgr/libsnapshot/test_helpers.cpp index b05123a23..9f1d6764f 100644 --- a/fs_mgr/libsnapshot/test_helpers.cpp +++ b/fs_mgr/libsnapshot/test_helpers.cpp @@ -227,7 +227,7 @@ AssertionResult LowSpaceUserdata::Init(uint64_t max_free_space) { return AssertionFailure() << "Temp file allocated to " << big_file_->path << ", not in " << kUserDataDevice; } - uint64_t next_consume = std::min(available_space_ - max_free_space, + uint64_t next_consume = std::min(std::max(available_space_, max_free_space) - max_free_space, (uint64_t)std::numeric_limits::max()); off_t allocated = 0; while (next_consume > 0 && free_space_ > max_free_space) { From bda181a86a89ebcd3d6eb1208d173bec05316e11 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Tue, 7 Feb 2023 05:22:22 +0000 Subject: [PATCH 2/2] libsnapshot: Fix crash in cow writer test due to missing ASSERT. Bug: N/A Test: vts_libsnapshot_test Change-Id: I72cf6f52fc15061da669d14cf5d334b44ad83501 --- fs_mgr/libsnapshot/libsnapshot_cow/cow_writer.cpp | 9 ++++----- fs_mgr/libsnapshot/snapshot_writer_test.cpp | 4 ++-- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/fs_mgr/libsnapshot/libsnapshot_cow/cow_writer.cpp b/fs_mgr/libsnapshot/libsnapshot_cow/cow_writer.cpp index 3932fad94..56b48f0c7 100644 --- a/fs_mgr/libsnapshot/libsnapshot_cow/cow_writer.cpp +++ b/fs_mgr/libsnapshot/libsnapshot_cow/cow_writer.cpp @@ -298,13 +298,12 @@ bool CowWriter::Initialize(borrowed_fd fd) { return false; } - bool ret = OpenForWrite(); - - if (ret) { - InitWorkers(); + if (!OpenForWrite()) { + return false; } - return ret; + InitWorkers(); + return true; } bool CowWriter::InitializeAppend(android::base::unique_fd&& fd, uint64_t label) { diff --git a/fs_mgr/libsnapshot/snapshot_writer_test.cpp b/fs_mgr/libsnapshot/snapshot_writer_test.cpp index da48eb933..a03632b46 100644 --- a/fs_mgr/libsnapshot/snapshot_writer_test.cpp +++ b/fs_mgr/libsnapshot/snapshot_writer_test.cpp @@ -33,8 +33,8 @@ TEST_F(CompressedSnapshotWriterTest, ReadAfterWrite) { TemporaryFile cow_device_file{}; android::snapshot::CowOptions options{.block_size = BLOCK_SIZE}; android::snapshot::CompressedSnapshotWriter snapshot_writer{options}; - snapshot_writer.SetCowDevice(android::base::unique_fd{cow_device_file.fd}); - snapshot_writer.Initialize(); + ASSERT_TRUE(snapshot_writer.SetCowDevice(android::base::unique_fd{cow_device_file.fd})); + ASSERT_TRUE(snapshot_writer.Initialize()); std::vector buffer; buffer.resize(BLOCK_SIZE); std::fill(buffer.begin(), buffer.end(), 123);