From 7a0fb3562fec4b4721e4ece6bfb80d76674a6c77 Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Wed, 19 Feb 2020 20:52:51 -0800 Subject: [PATCH 1/3] libsnapshot: delete InitiateMergeAndWait. Now that update_engine is responsible for initiating the merge, InitiateMergeAndWait function becomes useless. Bug: 147696014 Test: libsnapshot_test Change-Id: I5473dc543ca8ac2fd31f597720847b02d0e5e33d --- .../include/libsnapshot/snapshot.h | 10 -- fs_mgr/libsnapshot/snapshot.cpp | 62 -------- fs_mgr/libsnapshot/snapshot_test.cpp | 14 +- fs_mgr/libsnapshot/snapshotctl.cpp | 140 +----------------- 4 files changed, 15 insertions(+), 211 deletions(-) diff --git a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h index 32345d26f..3faa3848e 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h @@ -175,16 +175,6 @@ class SnapshotManager final { UpdateState ProcessUpdateState(const std::function& callback = {}, const std::function& before_cancel = {}); - // Initiate the merge if necessary, then wait for the merge to finish. - // See InitiateMerge() and ProcessUpdateState() for details. - // Returns: - // - None if no merge to initiate - // - Unverified if called on the source slot - // - MergeCompleted if merge is completed - // - other states indicating an error has occurred - UpdateState InitiateMergeAndWait(SnapshotMergeReport* report = nullptr, - const std::function& before_cancel = {}); - // Find the status of the current update, if any. // // |progress| depends on the returned status: diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index 7f9c7f1ff..3513db903 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -2493,68 +2493,6 @@ std::unique_ptr SnapshotManager::EnsureMetadataMounted() { return AutoUnmountDevice::New(device_->GetMetadataDir()); } -UpdateState SnapshotManager::InitiateMergeAndWait(SnapshotMergeReport* stats_report, - const std::function& before_cancel) { - { - auto lock = LockExclusive(); - // Sync update state from file with bootloader. - if (!WriteUpdateState(lock.get(), ReadUpdateState(lock.get()))) { - LOG(WARNING) << "Unable to sync write update state, fastboot may " - << "reject / accept wipes incorrectly!"; - } - } - - auto merge_stats = SnapshotMergeStats::GetInstance(*this); - - unsigned int last_progress = 0; - auto callback = [&]() -> bool { - double progress; - GetUpdateState(&progress); - if (last_progress < static_cast(progress)) { - last_progress = progress; - LOG(INFO) << "Waiting for merge to complete: " << last_progress << "%."; - } - return true; // continue - }; - - LOG(INFO) << "Waiting for any previous merge request to complete. " - << "This can take up to several minutes."; - merge_stats->Start(); - auto state = ProcessUpdateState(callback, before_cancel); - merge_stats->set_state(state); - if (state == UpdateState::None) { - LOG(INFO) << "Can't find any snapshot to merge."; - return state; - } - if (state == UpdateState::Unverified) { - if (GetCurrentSlot() != Slot::Target) { - LOG(INFO) << "Cannot merge until device reboots."; - return state; - } - - if (!InitiateMerge()) { - LOG(ERROR) << "Failed to initiate merge."; - return state; - } - // All other states can be handled by ProcessUpdateState. - LOG(INFO) << "Waiting for merge to complete. This can take up to several minutes."; - last_progress = 0; - state = ProcessUpdateState(callback, before_cancel); - merge_stats->set_state(state); - } - - LOG(INFO) << "Merge finished with state \"" << state << "\"."; - if (stats_report) { - auto result = merge_stats->Finish(); - if (result) { - *stats_report = result->report(); - } else { - LOG(WARNING) << "SnapshotMergeStatus::Finish failed."; - } - } - return state; -} - bool SnapshotManager::HandleImminentDataWipe(const std::function& callback) { if (!device_->IsRecovery()) { LOG(ERROR) << "Data wipes are only allowed in recovery."; diff --git a/fs_mgr/libsnapshot/snapshot_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp index 7d16ec280..855451d82 100644 --- a/fs_mgr/libsnapshot/snapshot_test.cpp +++ b/fs_mgr/libsnapshot/snapshot_test.cpp @@ -1027,7 +1027,8 @@ TEST_F(SnapshotUpdateTest, FullUpdateFlow) { } // Initiate the merge and wait for it to be completed. - ASSERT_EQ(UpdateState::MergeCompleted, init->InitiateMergeAndWait()); + ASSERT_TRUE(init->InitiateMerge()); + ASSERT_EQ(UpdateState::MergeCompleted, init->ProcessUpdateState()); // Check that the target partitions have the same content after the merge. for (const auto& name : {"sys_b", "vnd_b", "prd_b"}) { @@ -1201,7 +1202,8 @@ TEST_F(SnapshotUpdateTest, ReclaimCow) { // Initiate the merge and wait for it to be completed. auto new_sm = SnapshotManager::New(new TestDeviceInfo(fake_super, "_b")); - ASSERT_EQ(UpdateState::MergeCompleted, new_sm->InitiateMergeAndWait()); + ASSERT_TRUE(new_sm->InitiateMerge()); + ASSERT_EQ(UpdateState::MergeCompleted, new_sm->ProcessUpdateState()); // Execute the second update. ASSERT_TRUE(new_sm->BeginUpdate()); @@ -1341,7 +1343,8 @@ TEST_F(SnapshotUpdateTest, MergeCannotRemoveCow) { ASSERT_GE(fd, 0); // COW cannot be removed due to open fd, so expect a soft failure. - ASSERT_EQ(UpdateState::MergeNeedsReboot, init->InitiateMergeAndWait()); + ASSERT_TRUE(init->InitiateMerge()); + ASSERT_EQ(UpdateState::MergeNeedsReboot, init->ProcessUpdateState()); // Simulate shutting down the device. fd.reset(); @@ -1354,7 +1357,7 @@ TEST_F(SnapshotUpdateTest, MergeCannotRemoveCow) { ASSERT_FALSE(sm->IsSnapshotDevice("sys_b", nullptr)); // Merge should be able to complete now. - ASSERT_EQ(UpdateState::MergeCompleted, init->InitiateMergeAndWait()); + ASSERT_EQ(UpdateState::MergeCompleted, init->ProcessUpdateState()); } class MetadataMountedTest : public SnapshotUpdateTest { @@ -1691,7 +1694,8 @@ TEST_P(FlashAfterUpdateTest, FlashSlotAfterUpdate) { // There should be no snapshot to merge. auto new_sm = SnapshotManager::New(new TestDeviceInfo(fake_super, flashed_slot_suffix)); - ASSERT_EQ(UpdateState::Cancelled, new_sm->InitiateMergeAndWait()); + // update_enigne 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()); diff --git a/fs_mgr/libsnapshot/snapshotctl.cpp b/fs_mgr/libsnapshot/snapshotctl.cpp index aa5e9c1fa..a44de8419 100644 --- a/fs_mgr/libsnapshot/snapshotctl.cpp +++ b/fs_mgr/libsnapshot/snapshotctl.cpp @@ -24,12 +24,8 @@ #include #include #include -#include -#include -#include -#include -#include "utility.h" +#include using namespace std::string_literals; @@ -39,146 +35,22 @@ int Usage() { "Actions:\n" " dump\n" " Print snapshot states.\n" - " merge [--logcat] [--log-to-file] [--report] [--dry-run]\n" - " Initialize merge and wait for it to be completed.\n" - " If --logcat is specified, log to logcat.\n" - " If --log-to-file is specified, log to /data/misc/snapshotctl_log/.\n" - " If both specified, log to both. If none specified, log to stdout.\n" - " If --report is specified, send merge statistics to statsd.\n" - " If --dry-run flag, no real merge operation is is triggered, and\n" - " sample statistics are sent to statsd for testing purpose.\n"; + " merge\n" + " Deprecated.\n"; return EX_USAGE; } namespace android { namespace snapshot { -static SnapshotMergeReport GetDummySnapshotMergeReport() { - SnapshotMergeReport fake_report; - - fake_report.set_state(UpdateState::MergeCompleted); - fake_report.set_resume_count(56); - - return fake_report; -} - bool DumpCmdHandler(int /*argc*/, char** argv) { android::base::InitLogging(argv, &android::base::StderrLogger); return SnapshotManager::New()->Dump(std::cout); } -class FileLogger { - public: - FileLogger() { - static constexpr const char* kLogFilePath = "/data/misc/snapshotctl_log/"; - std::stringstream ss; - ss << kLogFilePath << "snapshotctl." << Now() << ".log"; - fd_.reset(TEMP_FAILURE_RETRY( - open(ss.str().c_str(), - O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC | O_NOFOLLOW | O_SYNC, 0644))); - if (fd_ == -1) { - PLOG(ERROR) << "Cannot open persistent log " << ss.str(); - return; - } - // Explicitly chmod again because mode in open() may be masked by umask. - if (fchmod(fd_.get(), 0644) == -1) { - PLOG(ERROR) << "Cannot chmod 0644 persistent log " << ss.str(); - return; - } - } - // Copy-contuctor needed to be converted to std::function. - FileLogger(const FileLogger& other) { fd_.reset(dup(other.fd_)); } - void operator()(android::base::LogId, android::base::LogSeverity, const char* /*tag*/, - const char* /*file*/, unsigned int /*line*/, const char* message) { - if (fd_ == -1) return; - std::stringstream ss; - ss << Now() << ":" << message << "\n"; - (void)android::base::WriteStringToFd(ss.str(), fd_); - } - - private: - android::base::unique_fd fd_; -}; - -class MergeCmdLogger { - public: - MergeCmdLogger(int argc, char** argv) { - for (int i = 0; i < argc; ++i) { - if (argv[i] == "--logcat"s) { - loggers_.push_back(android::base::LogdLogger()); - } - if (argv[i] == "--log-to-file"s) { - loggers_.push_back(std::move(FileLogger())); - } - } - if (loggers_.empty()) { - loggers_.push_back(&android::base::StdioLogger); - } - } - void operator()(android::base::LogId id, android::base::LogSeverity severity, const char* tag, - const char* file, unsigned int line, const char* message) { - for (auto&& logger : loggers_) { - logger(id, severity, tag, file, line, message); - } - } - - private: - std::vector loggers_; -}; - -bool MergeCmdHandler(int argc, char** argv) { - std::chrono::milliseconds passed_ms; - - bool report_to_statsd = false; - bool dry_run = false; - for (int i = 2; i < argc; ++i) { - if (argv[i] == "--report"s) { - report_to_statsd = true; - } else if (argv[i] == "--dry-run"s) { - dry_run = true; - } - } - - // 'snapshotctl merge' is stripped away from arguments to - // Logger. - android::base::InitLogging(argv); - android::base::SetLogger(MergeCmdLogger(argc - 2, argv + 2)); - - UpdateState state; - SnapshotMergeReport merge_report; - if (dry_run) { - merge_report = GetDummySnapshotMergeReport(); - state = merge_report.state(); - passed_ms = std::chrono::milliseconds(1234); - } else { - auto begin = std::chrono::steady_clock::now(); - - state = SnapshotManager::New()->InitiateMergeAndWait(&merge_report); - - // We could wind up in the Unverified state if the device rolled back or - // hasn't fully rebooted. Ignore this. - if (state == UpdateState::None || state == UpdateState::Unverified) { - return true; - } - - auto end = std::chrono::steady_clock::now(); - passed_ms = std::chrono::duration_cast(end - begin); - } - - if (report_to_statsd) { - android::util::stats_write(android::util::SNAPSHOT_MERGE_REPORTED, - static_cast(merge_report.state()), - static_cast(passed_ms.count()), - static_cast(merge_report.resume_count())); - } - - if (state == UpdateState::MergeCompleted) { - LOG(INFO) << "Snapshot merged in " << passed_ms.count() << " ms."; - return true; - } - - LOG(ERROR) << "Snapshot failed to merge with state \"" << state << "\"."; - +bool MergeCmdHandler(int /*argc*/, char** argv) { + android::base::InitLogging(argv, &android::base::StderrLogger); + LOG(WARNING) << "Deprecated. Call update_engine_client --merge instead."; return false; } From 4a64da427e1de06ea5dc9a2df463e970a4c28af2 Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Wed, 12 Feb 2020 18:20:07 +0000 Subject: [PATCH 2/3] Revert "libsnapshot callstack files readable by dumpstate" This reverts commit faa49d1d0052e074606d88cc56638b573d6e5fb0. Reason for revert: Callstack should not be logged. Test: none Bug: 148818798 Change-Id: Ie5506ae0c7408e255a464b2f403d0a47d272229e --- fs_mgr/libsnapshot/snapshot.cpp | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index 3513db903..613b71196 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -238,12 +238,7 @@ bool SnapshotManager::RemoveAllUpdateState(LockedFile* lock, const std::function LOG(WARNING) << callstack_str.c_str(); std::stringstream path; path << "/data/misc/snapshotctl_log/libsnapshot." << Now() << ".log"; - std::string path_str = path.str(); - android::base::WriteStringToFile(callstack_str.c_str(), path_str); - if (chmod(path_str.c_str(), 0644) == -1) { - PLOG(WARNING) << "Unable to chmod 0644 " - << ", file maybe dropped from bugreport:" << path_str; - } + android::base::WriteStringToFile(callstack_str.c_str(), path.str()); #endif if (!RemoveAllSnapshots(lock)) { From 78671597cad5618b8d7522ccf916026f74735c3c Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Wed, 12 Feb 2020 18:21:20 +0000 Subject: [PATCH 3/3] Revert "libsnapshot::RemoveAllUpdateState log stack" Revert submission snapshotctl_callstack Reason for revert: Callstack should not be logged Reverted Changes: Ib80c74a9a:Temporarily add libutilscallstack dependency I2dfb6b7f1:libsnapshot::RemoveAllUpdateState log stack Test: none Bug: 148818798 Change-Id: I34683e93f10971629737f6fe648b25c6066c702f --- fs_mgr/libsnapshot/Android.bp | 16 ---------------- fs_mgr/libsnapshot/snapshot.cpp | 19 ------------------- 2 files changed, 35 deletions(-) diff --git a/fs_mgr/libsnapshot/Android.bp b/fs_mgr/libsnapshot/Android.bp index 0a0a21ddb..d670ca064 100644 --- a/fs_mgr/libsnapshot/Android.bp +++ b/fs_mgr/libsnapshot/Android.bp @@ -96,16 +96,6 @@ cc_library_static { static_libs: [ "libfs_mgr_binder" ], - - shared_libs: [ - // TODO(b/148818798): remove when parent bug is fixed - "libutilscallstack", - ], - cflags: [ - "-g", - "-O0", - "-DLIBSNAPSHOT_USE_CALLSTACK", - ], } cc_library_static { @@ -179,9 +169,6 @@ cc_defaults { "libsparse", "libutils", "libz", - - // TODO(b/148818798): remove when parent bug is fixed - "libutilscallstack", ], static_libs: [ "libfs_mgr", @@ -231,8 +218,5 @@ cc_binary { "libprotobuf-cpp-lite", "libstatslog", "libutils", - - // TODO(b/148818798): remove when parent bug is fixed. - "libutilscallstack", ], } diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index 613b71196..c2f15e843 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -21,7 +21,6 @@ #include #include -#include #include #include @@ -38,10 +37,6 @@ #include #include -#ifdef LIBSNAPSHOT_USE_CALLSTACK -#include -#endif - #include #include #include "device_info.h" @@ -227,20 +222,6 @@ bool SnapshotManager::RemoveAllUpdateState(LockedFile* lock, const std::function LOG(INFO) << "Removing all update state."; -#ifdef LIBSNAPSHOT_USE_CALLSTACK - LOG(WARNING) << "Logging stack; see b/148818798."; - // Do not use CallStack's log functions because snapshotctl relies on - // android-base/logging to save log to files. - // TODO(b/148818798): remove this before we ship. - CallStack callstack; - callstack.update(); - auto callstack_str = callstack.toString(); - LOG(WARNING) << callstack_str.c_str(); - std::stringstream path; - path << "/data/misc/snapshotctl_log/libsnapshot." << Now() << ".log"; - android::base::WriteStringToFile(callstack_str.c_str(), path.str()); -#endif - if (!RemoveAllSnapshots(lock)) { LOG(ERROR) << "Could not remove all snapshots"; return false;