From f02cd78e96a96716c5bf3a3691bc79d1bd91cebe Mon Sep 17 00:00:00 2001 From: Nandana Dutt Date: Fri, 14 Jun 2019 14:25:13 +0100 Subject: Remove old dumpstate aidl methods In Q we added new aidl methods for dumpstate to communicate with SystemServer. The old methods can be removed now. This change is functionally equivalent to before except for how progress is reported. Dumpstate loads previous run stats and calculates expected run time. If the current run exceeds that time, it used to let the client know via onMaxProgressUpdated(). Given the API world, this CL simplifies this model so that dumpstate conveys just one piece of information, i.e. final progress precentage, rather than current, previous_max, and new_max. Test: atest dumpstate_test Test: bugreport from power button works as expected BUG: 128980174 Change-Id: Id9103649b0f7c8e6ea0b79583ea04825cb5b455f Merged-In: Id9103649b0f7c8e6ea0b79583ea04825cb5b455f --- cmds/dumpstate/Android.bp | 1 - cmds/dumpstate/DumpstateSectionReporter.cpp | 42 ------------- cmds/dumpstate/DumpstateSectionReporter.h | 65 ------------------- cmds/dumpstate/DumpstateService.cpp | 3 +- .../binder/android/os/IDumpstateListener.aidl | 17 ----- cmds/dumpstate/dumpstate.cpp | 25 ++------ cmds/dumpstate/dumpstate.h | 8 +-- cmds/dumpstate/tests/dumpstate_smoke_test.cpp | 21 ------- cmds/dumpstate/tests/dumpstate_test.cpp | 72 +++++----------------- 9 files changed, 22 insertions(+), 232 deletions(-) delete mode 100644 cmds/dumpstate/DumpstateSectionReporter.cpp delete mode 100644 cmds/dumpstate/DumpstateSectionReporter.h diff --git a/cmds/dumpstate/Android.bp b/cmds/dumpstate/Android.bp index 8d383f501a..93bbe902c0 100644 --- a/cmds/dumpstate/Android.bp +++ b/cmds/dumpstate/Android.bp @@ -93,7 +93,6 @@ cc_defaults { "libutils", ], srcs: [ - "DumpstateSectionReporter.cpp", "DumpstateService.cpp", ], static_libs: [ diff --git a/cmds/dumpstate/DumpstateSectionReporter.cpp b/cmds/dumpstate/DumpstateSectionReporter.cpp deleted file mode 100644 index f814bde26d..0000000000 --- a/cmds/dumpstate/DumpstateSectionReporter.cpp +++ /dev/null @@ -1,42 +0,0 @@ -/* - * Copyright (C) 2018 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#define LOG_TAG "dumpstate" - -#include "DumpstateSectionReporter.h" - -namespace android { -namespace os { -namespace dumpstate { - -DumpstateSectionReporter::DumpstateSectionReporter(const std::string& title, - sp listener, - bool sendReport) - : title_(title), listener_(listener), sendReport_(sendReport), status_(OK), size_(-1) { - started_ = std::chrono::steady_clock::now(); -} - -DumpstateSectionReporter::~DumpstateSectionReporter() { - if ((listener_ != nullptr) && (sendReport_)) { - auto elapsed = std::chrono::duration_cast( - std::chrono::steady_clock::now() - started_); - listener_->onSectionComplete(title_, status_, size_, (int32_t)elapsed.count()); - } -} - -} // namespace dumpstate -} // namespace os -} // namespace android diff --git a/cmds/dumpstate/DumpstateSectionReporter.h b/cmds/dumpstate/DumpstateSectionReporter.h deleted file mode 100644 index e971de84c5..0000000000 --- a/cmds/dumpstate/DumpstateSectionReporter.h +++ /dev/null @@ -1,65 +0,0 @@ -/* - * Copyright (C) 2018 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#ifndef ANDROID_OS_DUMPSTATESECTIONREPORTER_H_ -#define ANDROID_OS_DUMPSTATESECTIONREPORTER_H_ - -#include -#include - -namespace android { -namespace os { -namespace dumpstate { - - -/* - * Helper class used to report per section details to a listener. - * - * Typical usage: - * - * DumpstateSectionReporter sectionReporter(title, listener, sendReport); - * sectionReporter.setSize(5000); - * - */ -class DumpstateSectionReporter { - public: - DumpstateSectionReporter(const std::string& title, sp listener, - bool sendReport); - - ~DumpstateSectionReporter(); - - void setStatus(status_t status) { - status_ = status; - } - - void setSize(int size) { - size_ = size; - } - - private: - std::string title_; - android::sp listener_; - bool sendReport_; - status_t status_; - int size_; - std::chrono::time_point started_; -}; - -} // namespace dumpstate -} // namespace os -} // namespace android - -#endif // ANDROID_OS_DUMPSTATESECTIONREPORTER_H_ diff --git a/cmds/dumpstate/DumpstateService.cpp b/cmds/dumpstate/DumpstateService.cpp index 37ba4f906e..f98df99534 100644 --- a/cmds/dumpstate/DumpstateService.cpp +++ b/cmds/dumpstate/DumpstateService.cpp @@ -200,8 +200,7 @@ status_t DumpstateService::dump(int fd, const Vector&) { dprintf(fd, "id: %d\n", ds_->id_); dprintf(fd, "pid: %d\n", ds_->pid_); dprintf(fd, "update_progress: %s\n", ds_->options_->do_progress_updates ? "true" : "false"); - dprintf(fd, "update_progress_threshold: %d\n", ds_->update_progress_threshold_); - dprintf(fd, "last_updated_progress: %d\n", ds_->last_updated_progress_); + dprintf(fd, "last_percent_progress: %d\n", ds_->last_reported_percent_progress_); dprintf(fd, "progress:\n"); ds_->progress_->Dump(fd, " "); dprintf(fd, "args: %s\n", ds_->options_->args.c_str()); diff --git a/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl b/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl index ea1e467dca..e486460753 100644 --- a/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl +++ b/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl @@ -61,21 +61,4 @@ interface IDumpstateListener { * Called when taking bugreport finishes successfully. */ void onFinished(); - - // TODO(b/111441001): Remove old methods when not used anymore. - void onProgressUpdated(int progress); - void onMaxProgressUpdated(int maxProgress); - - /** - * Called after every section is complete. - * - * @param name section name - * @param status values from status_t - * {@code OK} section completed successfully - * {@code TIMEOUT} dump timed out - * {@code != OK} error - * @param size size in bytes, may be invalid if status != OK - * @param durationMs duration in ms - */ - void onSectionComplete(@utf8InCpp String name, int status, int size, int durationMs); } diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index bdb13634f2..b934047713 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -77,7 +77,6 @@ #include #include #include "DumpstateInternal.h" -#include "DumpstateSectionReporter.h" #include "DumpstateService.h" #include "dumpstate.h" @@ -102,7 +101,6 @@ using android::base::StringPrintf; using android::os::IDumpstateListener; using android::os::dumpstate::CommandOptions; using android::os::dumpstate::DumpFileToFd; -using android::os::dumpstate::DumpstateSectionReporter; using android::os::dumpstate::PropertiesHelper; // Keep in sync with @@ -1016,7 +1014,6 @@ static Dumpstate::RunStatus RunDumpsysTextByPriority(const std::string& title, i RETURN_IF_USER_DENIED_CONSENT(); std::string path(title); path.append(" - ").append(String8(service).c_str()); - DumpstateSectionReporter section_reporter(path, ds.listener_, ds.report_section_); size_t bytes_written = 0; status_t status = dumpsys.startDumpThread(service, args); if (status == OK) { @@ -1024,12 +1021,10 @@ static Dumpstate::RunStatus RunDumpsysTextByPriority(const std::string& title, i std::chrono::duration elapsed_seconds; status = dumpsys.writeDump(STDOUT_FILENO, service, service_timeout, /* as_proto = */ false, elapsed_seconds, bytes_written); - section_reporter.setSize(bytes_written); dumpsys.writeDumpFooter(STDOUT_FILENO, service, elapsed_seconds); bool dump_complete = (status == OK); dumpsys.stopDumpThread(dump_complete); } - section_reporter.setStatus(status); auto elapsed_duration = std::chrono::duration_cast( std::chrono::steady_clock::now() - start); @@ -1092,7 +1087,6 @@ static Dumpstate::RunStatus RunDumpsysProto(const std::string& title, int priori path.append("_HIGH"); } path.append(kProtoExt); - DumpstateSectionReporter section_reporter(path, ds.listener_, ds.report_section_); status_t status = dumpsys.startDumpThread(service, args); if (status == OK) { status = ds.AddZipEntryFromFd(path, dumpsys.getDumpFd(), service_timeout); @@ -1101,8 +1095,6 @@ static Dumpstate::RunStatus RunDumpsysProto(const std::string& title, int priori } ZipWriter::FileEntry file_entry; ds.zip_writer_->GetLastEntry(&file_entry); - section_reporter.setSize(file_entry.compressed_size); - section_reporter.setStatus(status); auto elapsed_duration = std::chrono::duration_cast( std::chrono::steady_clock::now() - start); @@ -2772,6 +2764,7 @@ int run_main(int argc, char* argv[]) { Dumpstate::Dumpstate(const std::string& version) : pid_(getpid()), options_(new Dumpstate::DumpOptions()), + last_reported_percent_progress_(0), version_(version), now_(time(nullptr)) { } @@ -3534,31 +3527,25 @@ void Dumpstate::UpdateProgress(int32_t delta_sec) { } // Always update progess so stats can be tuned... - bool max_changed = progress_->Inc(delta_sec); + progress_->Inc(delta_sec); // ...but only notifiy listeners when necessary. if (!options_->do_progress_updates) return; int progress = progress_->Get(); int max = progress_->GetMax(); + int percent = 100 * progress / max; - // adjusts max on the fly - if (max_changed && listener_ != nullptr) { - listener_->onMaxProgressUpdated(max); - } - - int32_t last_update_delta = progress - last_updated_progress_; - if (last_updated_progress_ > 0 && last_update_delta < update_progress_threshold_) { + if (last_reported_percent_progress_ > 0 && percent <= last_reported_percent_progress_) { return; } - last_updated_progress_ = progress; + last_reported_percent_progress_ = percent; if (control_socket_fd_ >= 0) { dprintf(control_socket_fd_, "PROGRESS:%d/%d\n", progress, max); fsync(control_socket_fd_); } - int percent = 100 * progress / max; if (listener_ != nullptr) { if (percent % 5 == 0) { // We don't want to spam logcat, so only log multiples of 5. @@ -3570,8 +3557,6 @@ void Dumpstate::UpdateProgress(int32_t delta_sec) { fprintf(stderr, "Setting progress (%s): %d/%d (%d%%)\n", listener_name_.c_str(), progress, max, percent); } - // TODO(b/111441001): Remove in favor of onProgress - listener_->onProgressUpdated(progress); listener_->onProgress(percent); } diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h index ae6a72171a..f137fc927a 100644 --- a/cmds/dumpstate/dumpstate.h +++ b/cmds/dumpstate/dumpstate.h @@ -400,12 +400,8 @@ class Dumpstate { // Runtime options. std::unique_ptr options_; - // How frequently the progess should be updated;the listener will only be notificated when the - // delta from the previous update is more than the threshold. - int32_t update_progress_threshold_ = 100; - - // Last progress that triggered a listener updated - int32_t last_updated_progress_; + // Last progress that was sent to the listener [0-100]. + int last_reported_percent_progress_ = 0; // Whether it should take an screenshot earlier in the process. bool do_early_screenshot_ = false; diff --git a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp index f7acaf1b9b..181046a7a7 100644 --- a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp @@ -167,26 +167,6 @@ class DumpstateListener : public BnDumpstateListener { return binder::Status::ok(); } - binder::Status onProgressUpdated(int32_t progress) override { - dprintf(out_fd_, "\rIn progress %d/%d", progress, max_progress_); - return binder::Status::ok(); - } - - binder::Status onMaxProgressUpdated(int32_t max_progress) override { - std::lock_guard lock(lock_); - max_progress_ = max_progress; - return binder::Status::ok(); - } - - binder::Status onSectionComplete(const ::std::string& name, int32_t, int32_t size_bytes, - int32_t) override { - std::lock_guard lock(lock_); - if (sections_.get() != nullptr) { - sections_->push_back({name, size_bytes}); - } - return binder::Status::ok(); - } - bool getIsFinished() { std::lock_guard lock(lock_); return is_finished_; @@ -199,7 +179,6 @@ class DumpstateListener : public BnDumpstateListener { private: int out_fd_; - int max_progress_ = 5000; int error_code_ = -1; bool is_finished_ = false; std::shared_ptr> sections_; diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp index 4e6b084ff6..cff1d439d9 100644 --- a/cmds/dumpstate/tests/dumpstate_test.cpp +++ b/cmds/dumpstate/tests/dumpstate_test.cpp @@ -62,10 +62,6 @@ class DumpstateListenerMock : public IDumpstateListener { MOCK_METHOD1(onProgress, binder::Status(int32_t progress)); MOCK_METHOD1(onError, binder::Status(int32_t error_code)); MOCK_METHOD0(onFinished, binder::Status()); - MOCK_METHOD1(onProgressUpdated, binder::Status(int32_t progress)); - MOCK_METHOD1(onMaxProgressUpdated, binder::Status(int32_t max_progress)); - MOCK_METHOD4(onSectionComplete, binder::Status(const ::std::string& name, int32_t status, - int32_t size, int32_t durationMs)); protected: MOCK_METHOD0(onAsBinder, IBinder*()); @@ -590,7 +586,6 @@ class DumpstateTest : public DumpstateBaseTest { SetDryRun(false); SetBuildType(android::base::GetProperty("ro.build.type", "(unknown)")); ds.progress_.reset(new Progress()); - ds.update_progress_threshold_ = 0; ds.options_.reset(new Dumpstate::DumpOptions()); } @@ -615,10 +610,9 @@ class DumpstateTest : public DumpstateBaseTest { return status; } - void SetProgress(long progress, long initial_max, long threshold = 0) { + void SetProgress(long progress, long initial_max) { + ds.last_reported_percent_progress_ = 0; ds.options_->do_progress_updates = true; - ds.update_progress_threshold_ = threshold; - ds.last_updated_progress_ = 0; ds.progress_.reset(new Progress(initial_max, progress, 1.2)); } @@ -796,73 +790,36 @@ TEST_F(DumpstateTest, RunCommandProgress) { ds.listener_name_ = "FoxMulder"; SetProgress(0, 30); - EXPECT_CALL(*listener, onProgressUpdated(20)); EXPECT_CALL(*listener, onProgress(66)); // 20/30 % EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(20).Build())); std::string progress_message = GetProgressMessage(ds.listener_name_, 20, 30); EXPECT_THAT(out, StrEq("stdout\n")); EXPECT_THAT(err, StrEq("stderr\n" + progress_message)); - EXPECT_CALL(*listener, onProgressUpdated(30)); - EXPECT_CALL(*listener, onProgress(100)); // 35/35 % - EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(10).Build())); - progress_message = GetProgressMessage(ds.listener_name_, 30, 30); - EXPECT_THAT(out, StrEq("stdout\n")); - EXPECT_THAT(err, StrEq("stderr\n" + progress_message)); - - // Run a command that will increase maximum timeout. - EXPECT_CALL(*listener, onProgressUpdated(31)); - EXPECT_CALL(*listener, onMaxProgressUpdated(37)); - EXPECT_CALL(*listener, onProgress(83)); // 31/37 % - EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(1).Build())); - progress_message = GetProgressMessage(ds.listener_name_, 31, 37, 30); // 20% increase + EXPECT_CALL(*listener, onProgress(80)); // 24/30 % + EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(4).Build())); + progress_message = GetProgressMessage(ds.listener_name_, 24, 30); EXPECT_THAT(out, StrEq("stdout\n")); EXPECT_THAT(err, StrEq("stderr\n" + progress_message)); // Make sure command ran while in dry_run is counted. SetDryRun(true); - EXPECT_CALL(*listener, onProgressUpdated(35)); - EXPECT_CALL(*listener, onProgress(94)); // 35/37 % - EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(4).Build())); - progress_message = GetProgressMessage(ds.listener_name_, 35, 37); + EXPECT_CALL(*listener, onProgress(90)); // 27/30 % + EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(3).Build())); + progress_message = GetProgressMessage(ds.listener_name_, 27, 30); EXPECT_THAT(out, IsEmpty()); EXPECT_THAT(err, StrEq(progress_message)); - ds.listener_.clear(); -} - -TEST_F(DumpstateTest, RunCommandProgressIgnoreThreshold) { - sp listener(new DumpstateListenerMock()); - ds.listener_ = listener; - ds.listener_name_ = "FoxMulder"; - SetProgress(0, 8, 5); // 8 max, 5 threshold - - // First update should always be sent. - EXPECT_CALL(*listener, onProgressUpdated(1)); - EXPECT_CALL(*listener, onProgress(12)); // 1/12 % - EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(1).Build())); - std::string progress_message = GetProgressMessage(ds.listener_name_, 1, 8); + SetDryRun(false); + EXPECT_CALL(*listener, onProgress(96)); // 29/30 % + EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(2).Build())); + progress_message = GetProgressMessage(ds.listener_name_, 29, 30); EXPECT_THAT(out, StrEq("stdout\n")); EXPECT_THAT(err, StrEq("stderr\n" + progress_message)); - // Fourth update should be ignored because it's between the threshold (5 -1 = 4 < 5). - EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(4).Build())); - EXPECT_THAT(out, StrEq("stdout\n")); - EXPECT_THAT(err, StrEq("stderr\n")); - - // Third update should be sent because it reaches threshold (6 - 1 = 5). - EXPECT_CALL(*listener, onProgressUpdated(6)); - EXPECT_CALL(*listener, onProgress(75)); // 6/8 % + EXPECT_CALL(*listener, onProgress(100)); // 30/30 % EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(1).Build())); - progress_message = GetProgressMessage(ds.listener_name_, 6, 8); - EXPECT_THAT(out, StrEq("stdout\n")); - EXPECT_THAT(err, StrEq("stderr\n" + progress_message)); - - // Fourth update should be ignored because it's between the threshold (9 - 6 = 3 < 5). - // But max update should be sent. - EXPECT_CALL(*listener, onMaxProgressUpdated(10)); // 9 * 120% = 10.8 = 10 - EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(3).Build())); - progress_message = GetProgressMessage(ds.listener_name_, 9, 10, 8, false); + progress_message = GetProgressMessage(ds.listener_name_, 30, 30); EXPECT_THAT(out, StrEq("stdout\n")); EXPECT_THAT(err, StrEq("stderr\n" + progress_message)); @@ -1090,7 +1047,6 @@ TEST_F(DumpstateTest, DumpFileUpdateProgress) { ds.listener_name_ = "FoxMulder"; SetProgress(0, 30); - EXPECT_CALL(*listener, onProgressUpdated(5)); EXPECT_CALL(*listener, onProgress(16)); // 5/30 % EXPECT_EQ(0, DumpFile("", kTestDataPath + "single-line.txt")); -- cgit v1.2.3-59-g8ed1b