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
diff --git a/cmds/dumpstate/Android.bp b/cmds/dumpstate/Android.bp
index 8d383f5..93bbe90 100644
--- a/cmds/dumpstate/Android.bp
+++ b/cmds/dumpstate/Android.bp
@@ -93,7 +93,6 @@
"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 f814bde..0000000
--- 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<android::os::IDumpstateListener> 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::milliseconds>(
- 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 e971de8..0000000
--- 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 <android/os/IDumpstateListener.h>
-#include <utils/StrongPointer.h>
-
-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<android::os::IDumpstateListener> listener,
- bool sendReport);
-
- ~DumpstateSectionReporter();
-
- void setStatus(status_t status) {
- status_ = status;
- }
-
- void setSize(int size) {
- size_ = size;
- }
-
- private:
- std::string title_;
- android::sp<android::os::IDumpstateListener> listener_;
- bool sendReport_;
- status_t status_;
- int size_;
- std::chrono::time_point<std::chrono::steady_clock> 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 37ba4f9..f98df99 100644
--- a/cmds/dumpstate/DumpstateService.cpp
+++ b/cmds/dumpstate/DumpstateService.cpp
@@ -200,8 +200,7 @@
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 ea1e467..e486460 100644
--- a/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl
+++ b/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl
@@ -61,21 +61,4 @@
* 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 fe55fe7..6b2e3b7 100644
--- a/cmds/dumpstate/dumpstate.cpp
+++ b/cmds/dumpstate/dumpstate.cpp
@@ -80,7 +80,6 @@
#include <serviceutils/PriorityDumper.h>
#include <utils/StrongPointer.h>
#include "DumpstateInternal.h"
-#include "DumpstateSectionReporter.h"
#include "DumpstateService.h"
#include "dumpstate.h"
@@ -105,7 +104,6 @@
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
@@ -1047,7 +1045,6 @@
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) {
@@ -1055,12 +1052,10 @@
std::chrono::duration<double> 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::milliseconds>(
std::chrono::steady_clock::now() - start);
@@ -1123,7 +1118,6 @@
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);
@@ -1132,8 +1126,6 @@
}
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::milliseconds>(
std::chrono::steady_clock::now() - start);
@@ -2813,6 +2805,7 @@
Dumpstate::Dumpstate(const std::string& version)
: pid_(getpid()),
options_(new Dumpstate::DumpOptions()),
+ last_reported_percent_progress_(0),
version_(version),
now_(time(nullptr)) {
}
@@ -3575,31 +3568,25 @@
}
// 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.
@@ -3611,8 +3598,6 @@
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 fe330df..77b5e8a 100644
--- a/cmds/dumpstate/dumpstate.h
+++ b/cmds/dumpstate/dumpstate.h
@@ -405,12 +405,8 @@
// Runtime options.
std::unique_ptr<DumpOptions> 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 f7acaf1..181046a 100644
--- a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp
+++ b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp
@@ -167,26 +167,6 @@
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<std::mutex> 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<std::mutex> lock(lock_);
- if (sections_.get() != nullptr) {
- sections_->push_back({name, size_bytes});
- }
- return binder::Status::ok();
- }
-
bool getIsFinished() {
std::lock_guard<std::mutex> lock(lock_);
return is_finished_;
@@ -199,7 +179,6 @@
private:
int out_fd_;
- int max_progress_ = 5000;
int error_code_ = -1;
bool is_finished_ = false;
std::shared_ptr<std::vector<SectionInfo>> sections_;
diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp
index 4e6b084..cff1d43 100644
--- a/cmds/dumpstate/tests/dumpstate_test.cpp
+++ b/cmds/dumpstate/tests/dumpstate_test.cpp
@@ -62,10 +62,6 @@
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 @@
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 @@
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 @@
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<DumpstateListenerMock> 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 @@
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"));