Deprecated system properties used to update progress:
- Uses just the binder listener.
- Don't send all updates to the listener.
- SetListener returns a token that can be used to watch for dumpstate death.
Bug: 31636879
Test: dumpstate_test passes
Change-Id: Ie73fa355809b3b628ee39d7c52ded4b99387b14d
diff --git a/cmds/dumpstate/Android.mk b/cmds/dumpstate/Android.mk
index 3987fce..e11bf30 100644
--- a/cmds/dumpstate/Android.mk
+++ b/cmds/dumpstate/Android.mk
@@ -58,8 +58,9 @@
LOCAL_AIDL_INCLUDES := $(LOCAL_PATH)/binder
LOCAL_C_INCLUDES := $(LOCAL_PATH)/binder
LOCAL_SRC_FILES := \
+ binder/android/os/IDumpstate.aidl \
binder/android/os/IDumpstateListener.aidl \
- binder/android/os/IDumpstate.aidl
+ binder/android/os/IDumpstateToken.aidl
include $(BUILD_SHARED_LIBRARY)
diff --git a/cmds/dumpstate/DumpstateService.cpp b/cmds/dumpstate/DumpstateService.cpp
index a2a9018..f2dcf2b 100644
--- a/cmds/dumpstate/DumpstateService.cpp
+++ b/cmds/dumpstate/DumpstateService.cpp
@@ -25,6 +25,10 @@
namespace android {
namespace os {
+namespace {
+class DumpstateToken : public BnDumpstateToken {};
+}
+
DumpstateService::DumpstateService() : ds_(Dumpstate::GetInstance()) {
}
@@ -45,32 +49,36 @@
}
binder::Status DumpstateService::setListener(const std::string& name,
- const sp<IDumpstateListener>& listener, bool* set) {
+ const sp<IDumpstateListener>& listener,
+ sp<IDumpstateToken>* returned_token) {
+ *returned_token = nullptr;
if (name.empty()) {
MYLOGE("setListener(): name not set\n");
- *set = false;
return binder::Status::ok();
}
if (listener == nullptr) {
MYLOGE("setListener(): listener not set\n");
- *set = false;
return binder::Status::ok();
}
std::lock_guard<std::mutex> lock(lock_);
if (ds_.listener_ != nullptr) {
MYLOGE("setListener(%s): already set (%s)\n", name.c_str(), ds_.listener_name_.c_str());
- *set = false;
return binder::Status::ok();
}
+
ds_.listener_name_ = name;
ds_.listener_ = listener;
- *set = true;
+ *returned_token = new DumpstateToken();
+
return binder::Status::ok();
}
status_t DumpstateService::dump(int fd, const Vector<String16>&) {
dprintf(fd, "id: %d\n", ds_.id_);
dprintf(fd, "pid: %d\n", ds_.pid_);
+ dprintf(fd, "update_progress: %s\n", ds_.update_progress_ ? "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, "progress:\n");
ds_.progress_->Dump(fd, " ");
dprintf(fd, "args: %s\n", ds_.args_.c_str());
diff --git a/cmds/dumpstate/DumpstateService.h b/cmds/dumpstate/DumpstateService.h
index 544a7b6..4352d3d 100644
--- a/cmds/dumpstate/DumpstateService.h
+++ b/cmds/dumpstate/DumpstateService.h
@@ -23,6 +23,7 @@
#include <binder/BinderService.h>
#include "android/os/BnDumpstate.h"
+#include "android/os/BnDumpstateToken.h"
#include "dumpstate.h"
namespace android {
@@ -37,7 +38,7 @@
status_t dump(int fd, const Vector<String16>& args) override;
binder::Status setListener(const std::string& name, const sp<IDumpstateListener>& listener,
- bool* set) override;
+ sp<IDumpstateToken>* returned_token) override;
private:
Dumpstate& ds_;
diff --git a/cmds/dumpstate/binder/android/os/IDumpstate.aidl b/cmds/dumpstate/binder/android/os/IDumpstate.aidl
index e585a0e..4becccf 100644
--- a/cmds/dumpstate/binder/android/os/IDumpstate.aidl
+++ b/cmds/dumpstate/binder/android/os/IDumpstate.aidl
@@ -17,6 +17,7 @@
package android.os;
import android.os.IDumpstateListener;
+import android.os.IDumpstateToken;
/**
* Binder interface for the currently running dumpstate process.
@@ -25,9 +26,10 @@
interface IDumpstate {
/*
- * Sets the listener for dumpstate progress.
+ * Sets the listener for this dumpstate progress.
*
- * Returns true if the listener was set (it's like a Highlander: There Can be Only One).
+ * Returns a token used to monitor dumpstate death, or `nullptr` if the listener was already
+ * set (the listener behaves like a Highlander: There Can be Only One).
*/
- boolean setListener(@utf8InCpp String name, IDumpstateListener listener);
+ IDumpstateToken setListener(@utf8InCpp String name, IDumpstateListener listener);
}
diff --git a/cmds/dumpstate/binder/android/os/IDumpstateToken.aidl b/cmds/dumpstate/binder/android/os/IDumpstateToken.aidl
new file mode 100644
index 0000000..7f74ceb
--- /dev/null
+++ b/cmds/dumpstate/binder/android/os/IDumpstateToken.aidl
@@ -0,0 +1,24 @@
+/**
+ * Copyright (c) 2016, 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.
+ */
+
+package android.os;
+
+/**
+ * Token used by the IDumpstateListener to watch for dumpstate death.
+ * {@hide}
+ */
+interface IDumpstateToken {
+}
diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h
index 9124f94..3d3d7ed 100644
--- a/cmds/dumpstate/dumpstate.h
+++ b/cmds/dumpstate/dumpstate.h
@@ -390,6 +390,13 @@
// Whether progress updates should be published.
bool update_progress_ = false;
+ // 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_;
+
// Whether it should take an screenshot earlier in the process.
bool do_early_screenshot_ = false;
diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp
index 2fcf321..0d68901 100644
--- a/cmds/dumpstate/tests/dumpstate_test.cpp
+++ b/cmds/dumpstate/tests/dumpstate_test.cpp
@@ -38,7 +38,9 @@
using namespace android;
using ::testing::EndsWith;
+using ::testing::IsNull;
using ::testing::IsEmpty;
+using ::testing::NotNull;
using ::testing::StrEq;
using ::testing::StartsWith;
using ::testing::Test;
@@ -49,6 +51,7 @@
using os::DumpstateService;
using os::IDumpstateListener;
+using os::IDumpstateToken;
// Not used on test cases yet...
void dumpstate_board(void) {
@@ -106,6 +109,7 @@
SetBuildType(android::base::GetProperty("ro.build.type", "(unknown)"));
ds.progress_.reset(new Progress());
ds.update_progress_ = false;
+ ds.update_progress_threshold_ = 0;
}
// Runs a command and capture `stdout` and `stderr`.
@@ -149,55 +153,32 @@
ASSERT_EQ(2000, (int)uid);
}
- void SetProgress(long progress, long initial_max) {
+ void SetProgress(long progress, long initial_max, long threshold = 0) {
ds.update_progress_ = true;
+ ds.update_progress_threshold_ = threshold;
+ ds.last_updated_progress_ = 0;
ds.progress_.reset(new Progress(initial_max, progress, 1.2));
}
- // TODO: remove when progress is set by Binder callbacks.
- void AssertSystemProperty(const std::string& key, const std::string& expected_value) {
- std::string actualValue = android::base::GetProperty(key, "not set");
- EXPECT_THAT(expected_value, StrEq(actualValue)) << "invalid value for property " << key;
- }
-
- // TODO: remove when progress is set by Binder callbacks.
- std::string GetProgressMessageAndAssertSystemProperties(int progress, int max, int old_max = 0) {
- EXPECT_EQ(progress, ds.progress_->Get()) << "invalid progress";
- EXPECT_EQ(max, ds.progress_->GetMax()) << "invalid max";
-
- AssertSystemProperty(android::base::StringPrintf("dumpstate.%d.progress", getpid()),
- std::to_string(progress));
-
- bool max_increased = old_max > 0;
-
- std::string adjustment_message = "";
- if (max_increased) {
- AssertSystemProperty(android::base::StringPrintf("dumpstate.%d.max", getpid()),
- std::to_string(max));
- adjustment_message =
- android::base::StringPrintf("Adjusting max progress from %d to %d\n", old_max, max);
- }
-
- return android::base::StringPrintf("%sSetting progress (dumpstate.%d.progress): %d/%d\n",
- adjustment_message.c_str(), getpid(), progress, max);
- }
-
std::string GetProgressMessage(const std::string& listener_name, int progress, int max,
- int old_max = 0) {
+ int old_max = 0, bool update_progress = true) {
EXPECT_EQ(progress, ds.progress_->Get()) << "invalid progress";
EXPECT_EQ(max, ds.progress_->GetMax()) << "invalid max";
bool max_increased = old_max > 0;
- std::string adjustment_message = "";
+ std::string message = "";
if (max_increased) {
- adjustment_message =
+ message =
android::base::StringPrintf("Adjusting max progress from %d to %d\n", old_max, max);
}
- return android::base::StringPrintf("%sSetting progress (%s): %d/%d\n",
- adjustment_message.c_str(), listener_name.c_str(),
- progress, max);
+ if (update_progress) {
+ message += android::base::StringPrintf("Setting progress (%s): %d/%d\n",
+ listener_name.c_str(), progress, max);
+ }
+
+ return message;
}
// `stdout` and `stderr` from the last command ran.
@@ -346,33 +327,6 @@
" --pid --sleep 20' failed: killed by signal 15\n"));
}
-TEST_F(DumpstateTest, RunCommandProgressNoListener) {
- SetProgress(0, 30);
-
- EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(20).Build()));
- std::string progress_message = GetProgressMessageAndAssertSystemProperties(20, 30);
- EXPECT_THAT(out, StrEq("stdout\n"));
- EXPECT_THAT(err, StrEq("stderr\n" + progress_message));
-
- EXPECT_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(10).Build()));
- progress_message = GetProgressMessageAndAssertSystemProperties(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_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(1).Build()));
- progress_message = GetProgressMessageAndAssertSystemProperties(31, 37, 30); // 20% increase
- 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_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(4).Build()));
- progress_message = GetProgressMessageAndAssertSystemProperties(35, 37);
- EXPECT_THAT(out, IsEmpty());
- EXPECT_THAT(err, StrEq(progress_message));
-}
-
TEST_F(DumpstateTest, RunCommandProgress) {
sp<DumpstateListenerMock> listener(new DumpstateListenerMock());
ds.listener_ = listener;
@@ -410,6 +364,42 @@
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_EQ(0, RunCommand("", {kSimpleCommand}, CommandOptions::WithTimeout(1).Build()));
+ std::string progress_message = GetProgressMessage(ds.listener_name_, 1, 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 (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_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);
+ EXPECT_THAT(out, StrEq("stdout\n"));
+ EXPECT_THAT(err, StrEq("stderr\n" + progress_message));
+
+ ds.listener_.clear();
+}
+
TEST_F(DumpstateTest, RunCommandDropRoot) {
// First check root case - only available when running with 'adb root'.
uid_t uid = getuid();
@@ -499,18 +489,6 @@
EXPECT_THAT(err, IsEmpty());
}
-TEST_F(DumpstateTest, DumpFileUpdateProgressNoListener) {
- SetProgress(0, 30);
-
- EXPECT_EQ(0, DumpFile("", kTestDataPath + "single-line.txt"));
-
- std::string progress_message =
- GetProgressMessageAndAssertSystemProperties(5, 30); // TODO: unhardcode WEIGHT_FILE (5)?
-
- EXPECT_THAT(err, StrEq(progress_message));
- EXPECT_THAT(out, StrEq("I AM LINE1\n")); // dumpstate adds missing newline
-}
-
TEST_F(DumpstateTest, DumpFileUpdateProgress) {
sp<DumpstateListenerMock> listener(new DumpstateListenerMock());
ds.listener_ = listener;
@@ -535,27 +513,28 @@
TEST_F(DumpstateServiceTest, SetListenerNoName) {
sp<DumpstateListenerMock> listener(new DumpstateListenerMock());
- bool result;
- EXPECT_TRUE(dss.setListener("", listener, &result).isOk());
- EXPECT_FALSE(result);
+ sp<IDumpstateToken> token;
+ EXPECT_TRUE(dss.setListener("", listener, &token).isOk());
+ ASSERT_THAT(token, IsNull());
}
TEST_F(DumpstateServiceTest, SetListenerNoPointer) {
- bool result;
- EXPECT_TRUE(dss.setListener("whatever", nullptr, &result).isOk());
- EXPECT_FALSE(result);
+ sp<IDumpstateToken> token;
+ EXPECT_TRUE(dss.setListener("whatever", nullptr, &token).isOk());
+ ASSERT_THAT(token, IsNull());
}
TEST_F(DumpstateServiceTest, SetListenerTwice) {
sp<DumpstateListenerMock> listener(new DumpstateListenerMock());
- bool result;
- EXPECT_TRUE(dss.setListener("whatever", listener, &result).isOk());
- EXPECT_TRUE(result);
-
+ sp<IDumpstateToken> token;
+ EXPECT_TRUE(dss.setListener("whatever", listener, &token).isOk());
+ ASSERT_THAT(token, NotNull());
EXPECT_THAT(Dumpstate::GetInstance().listener_name_, StrEq("whatever"));
- EXPECT_TRUE(dss.setListener("whatever", listener, &result).isOk());
- EXPECT_FALSE(result);
+ token.clear();
+ EXPECT_TRUE(dss.setListener("whatsoever", listener, &token).isOk());
+ ASSERT_THAT(token, IsNull());
+ EXPECT_THAT(Dumpstate::GetInstance().listener_name_, StrEq("whatever"));
}
class ProgressTest : public DumpstateBaseTest {
diff --git a/cmds/dumpstate/utils.cpp b/cmds/dumpstate/utils.cpp
index d33460c..b5f328d 100644
--- a/cmds/dumpstate/utils.cpp
+++ b/cmds/dumpstate/utils.cpp
@@ -1451,27 +1451,19 @@
// ...but only notifiy listeners when necessary.
if (!update_progress_) return;
- // TODO: remove property support once Shell uses IDumpstateListener
- char key[PROPERTY_KEY_MAX];
- char value[PROPERTY_VALUE_MAX];
+ int progress = progress_->Get();
+ int max = progress_->GetMax();
// adjusts max on the fly
- if (max_changed) {
- if (listener_ != nullptr) {
- listener_->onMaxProgressUpdated(progress_->GetMax());
- } else {
- snprintf(key, sizeof(key), "dumpstate.%d.max", pid_);
- snprintf(value, sizeof(value), "%d", progress_->GetMax());
- int status = property_set(key, value);
- if (status != 0) {
- MYLOGE("Could not update max weight by setting system property %s to %s: %d\n", key,
- value, status);
- }
- }
+ if (max_changed && listener_ != nullptr) {
+ listener_->onMaxProgressUpdated(max);
}
- int32_t progress = progress_->Get();
- int32_t max = progress_->GetMax();
+ int32_t last_update_delta = progress - last_updated_progress_;
+ if (last_updated_progress_ > 0 && last_update_delta < update_progress_threshold_) {
+ return;
+ }
+ last_updated_progress_ = progress;
if (control_socket_fd_ >= 0) {
dprintf(control_socket_fd_, "PROGRESS:%d/%d\n", progress, max);
@@ -1488,24 +1480,6 @@
fprintf(stderr, "Setting progress (%s): %d/%d\n", listener_name_.c_str(), progress, max);
}
listener_->onProgressUpdated(progress);
- } else {
- snprintf(key, sizeof(key), "dumpstate.%d.progress", pid_);
- snprintf(value, sizeof(value), "%d", progress);
-
- if (progress % 100 == 0) {
- // We don't want to spam logcat, so only log multiples of 100.
- MYLOGD("Setting progress (%s): %s/%d\n", key, value, max);
- } else {
- // stderr is ignored on normal invocations, but useful when calling
- // /system/bin/dumpstate directly for debuggging.
- fprintf(stderr, "Setting progress (%s): %s/%d\n", key, value, max);
- }
-
- int status = property_set(key, value);
- if (status) {
- MYLOGE("Could not update progress by setting system property %s to %s: %d\n", key,
- value, status);
- }
}
}