diff --git a/fs_mgr/libfiemap/image_manager.cpp b/fs_mgr/libfiemap/image_manager.cpp index 841f21564..44f659b5d 100644 --- a/fs_mgr/libfiemap/image_manager.cpp +++ b/fs_mgr/libfiemap/image_manager.cpp @@ -16,6 +16,8 @@ #include +#include + #include #include #include @@ -574,7 +576,7 @@ bool ImageManager::UnmapImageDevice(const std::string& name, bool force) { return false; } auto& dm = DeviceMapper::Instance(); - LoopControl loop; + std::optional loop; std::string status; auto status_file = GetStatusFilePath(name); @@ -598,9 +600,14 @@ bool ImageManager::UnmapImageDevice(const std::string& name, bool force) { return false; } } else if (pieces[0] == "loop") { + // Lazily connect to loop-control to avoid spurious errors in recovery. + if (!loop.has_value()) { + loop.emplace(); + } + // Failure to remove a loop device is not fatal, since we can still // remove the backing file if we want. - loop.Detach(pieces[1]); + loop->Detach(pieces[1]); } else { LOG(ERROR) << "Unknown status: " << pieces[0]; } diff --git a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h index 0d90f6cd6..a79a86ddd 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h @@ -694,8 +694,8 @@ class SnapshotManager final : public ISnapshotManager { // Call ProcessUpdateState and handle states with special rules before data wipe. Specifically, // if |allow_forward_merge| and allow-forward-merge indicator exists, initiate merge if // necessary. - bool ProcessUpdateStateOnDataWipe(bool allow_forward_merge, - const std::function& callback); + UpdateState ProcessUpdateStateOnDataWipe(bool allow_forward_merge, + const std::function& callback); // Return device string of a mapped image, or if it is not available, the mapped image path. bool GetMappedImageDeviceStringOrPath(const std::string& device_name, diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index eb3a50144..cc2599d1f 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -894,6 +894,8 @@ UpdateState SnapshotManager::ProcessUpdateState(const std::function& cal const std::function& before_cancel) { while (true) { UpdateState state = CheckMergeState(before_cancel); + LOG(INFO) << "ProcessUpdateState handling state: " << state; + if (state == UpdateState::MergeFailed) { AcknowledgeMergeFailure(); } @@ -920,13 +922,15 @@ UpdateState SnapshotManager::CheckMergeState(const std::function& before } UpdateState state = CheckMergeState(lock.get(), before_cancel); + LOG(INFO) << "CheckMergeState for snapshots returned: " << state; + if (state == UpdateState::MergeCompleted) { // Do this inside the same lock. Failures get acknowledged without the // lock, because flock() might have failed. AcknowledgeMergeSuccess(lock.get()); } else if (state == UpdateState::Cancelled) { - if (!RemoveAllUpdateState(lock.get(), before_cancel)) { - return ReadSnapshotUpdateStatus(lock.get()).state(); + if (!device_->IsRecovery() && !RemoveAllUpdateState(lock.get(), before_cancel)) { + LOG(ERROR) << "Failed to remove all update state after acknowleding cancelled update."; } } return state; @@ -968,13 +972,23 @@ UpdateState SnapshotManager::CheckMergeState(LockedFile* lock, return UpdateState::MergeFailed; } + auto other_suffix = device_->GetOtherSlotSuffix(); + bool cancelled = false; bool failed = false; bool merging = false; bool needs_reboot = false; bool wrong_phase = false; for (const auto& snapshot : snapshots) { + if (android::base::EndsWith(snapshot, other_suffix)) { + // This will have triggered an error message in InitiateMerge already. + LOG(INFO) << "Skipping merge validation of unexpected snapshot: " << snapshot; + continue; + } + UpdateState snapshot_state = CheckTargetMergeState(lock, snapshot, update_status); + LOG(INFO) << "CheckTargetMergeState for " << snapshot << " returned: " << snapshot_state; + switch (snapshot_state) { case UpdateState::MergeFailed: failed = true; @@ -1173,7 +1187,7 @@ void SnapshotManager::AcknowledgeMergeSuccess(LockedFile* lock) { // indicator that cleanup is needed on reboot. If a factory data reset // was requested, it doesn't matter, everything will get wiped anyway. // To make testing easier we consider a /data wipe as cleaned up. - if (device_->IsRecovery() && !in_factory_data_reset_) { + if (device_->IsRecovery()) { WriteUpdateState(lock, UpdateState::MergeCompleted); return; } @@ -1692,6 +1706,7 @@ UpdateState SnapshotManager::GetUpdateState(double* progress) { for (const auto& snapshot : snapshots) { DmTargetSnapshot::Status current_status; + if (!IsSnapshotDevice(snapshot)) continue; if (!QuerySnapshotStatus(snapshot, nullptr, ¤t_status)) continue; fake_snapshots_status.sectors_allocated += current_status.sectors_allocated; @@ -3212,10 +3227,11 @@ bool SnapshotManager::HandleImminentDataWipe(const std::function& callba }; in_factory_data_reset_ = true; - bool ok = ProcessUpdateStateOnDataWipe(true /* allow_forward_merge */, process_callback); + UpdateState state = + ProcessUpdateStateOnDataWipe(true /* allow_forward_merge */, process_callback); in_factory_data_reset_ = false; - if (!ok) { + if (state == UpdateState::MergeFailed) { return false; } @@ -3223,6 +3239,16 @@ bool SnapshotManager::HandleImminentDataWipe(const std::function& callba if (!UnmapAllPartitionsInRecovery()) { LOG(ERROR) << "Unable to unmap all partitions; fastboot may fail to flash."; } + + if (state != UpdateState::None) { + auto lock = LockExclusive(); + if (!lock) return false; + + // Zap the update state so the bootloader doesn't think we're still + // merging. It's okay if this fails, it's informative only at this + // point. + WriteUpdateState(lock.get(), UpdateState::None); + } return true; } @@ -3257,15 +3283,15 @@ bool SnapshotManager::FinishMergeInRecovery() { return true; } -bool SnapshotManager::ProcessUpdateStateOnDataWipe(bool allow_forward_merge, - const std::function& callback) { +UpdateState SnapshotManager::ProcessUpdateStateOnDataWipe(bool allow_forward_merge, + const std::function& callback) { auto slot_number = SlotNumberForSlotSuffix(device_->GetSlotSuffix()); UpdateState state = ProcessUpdateState(callback); LOG(INFO) << "Update state in recovery: " << state; switch (state) { case UpdateState::MergeFailed: LOG(ERROR) << "Unrecoverable merge failure detected."; - return false; + return state; case UpdateState::Unverified: { // If an OTA was just applied but has not yet started merging: // @@ -3285,8 +3311,12 @@ bool SnapshotManager::ProcessUpdateStateOnDataWipe(bool allow_forward_merge, if (allow_forward_merge && access(GetForwardMergeIndicatorPath().c_str(), F_OK) == 0) { LOG(INFO) << "Forward merge allowed, initiating merge now."; - return InitiateMerge() && - ProcessUpdateStateOnDataWipe(false /* allow_forward_merge */, callback); + + if (!InitiateMerge()) { + LOG(ERROR) << "Failed to initiate merge on data wipe."; + return UpdateState::MergeFailed; + } + return ProcessUpdateStateOnDataWipe(false /* allow_forward_merge */, callback); } LOG(ERROR) << "Reverting to old slot since update will be deleted."; @@ -3304,7 +3334,7 @@ bool SnapshotManager::ProcessUpdateStateOnDataWipe(bool allow_forward_merge, default: break; } - return true; + return state; } bool SnapshotManager::EnsureNoOverflowSnapshot(LockedFile* lock) { diff --git a/fs_mgr/libsnapshot/snapshot_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp index d57aa6c22..bde4ccadd 100644 --- a/fs_mgr/libsnapshot/snapshot_test.cpp +++ b/fs_mgr/libsnapshot/snapshot_test.cpp @@ -636,8 +636,8 @@ TEST_F(SnapshotTest, FlashSuperDuringMerge) { // Because the status is Merging, we must call ProcessUpdateState, which should // detect a cancelled update. - ASSERT_EQ(sm->ProcessUpdateState(), UpdateState::Cancelled); - ASSERT_EQ(sm->GetUpdateState(), UpdateState::None); + ASSERT_EQ(init->ProcessUpdateState(), UpdateState::Cancelled); + ASSERT_EQ(init->GetUpdateState(), UpdateState::None); } TEST_F(SnapshotTest, UpdateBootControlHal) { @@ -1767,7 +1767,7 @@ TEST_F(SnapshotUpdateTest, DataWipeRollbackInRecovery) { ASSERT_TRUE(new_sm->HandleImminentDataWipe()); // Manually mount metadata so that we can call GetUpdateState() below. MountMetadata(); - EXPECT_EQ(new_sm->GetUpdateState(), UpdateState::Unverified); + EXPECT_EQ(new_sm->GetUpdateState(), UpdateState::None); EXPECT_TRUE(test_device->IsSlotUnbootable(1)); EXPECT_FALSE(test_device->IsSlotUnbootable(0)); } @@ -2105,8 +2105,12 @@ TEST_P(FlashAfterUpdateTest, FlashSlotAfterUpdate) { // There should be no snapshot to merge. auto new_sm = SnapshotManager::New(new TestDeviceInfo(fake_super, flashed_slot_suffix)); - // update_enigne calls ProcessUpdateState first -- should see Cancelled. - ASSERT_EQ(UpdateState::Cancelled, new_sm->ProcessUpdateState()); + if (flashed_slot == 0 && after_merge) { + ASSERT_EQ(UpdateState::MergeCompleted, new_sm->ProcessUpdateState()); + } else { + // update_engine calls ProcessUpdateState first -- should see Cancelled. + ASSERT_EQ(UpdateState::Cancelled, new_sm->ProcessUpdateState()); + } // Next OTA calls CancelUpdate no matter what. ASSERT_TRUE(new_sm->CancelUpdate());