dumpstate: notes on usage
This is what a few old maintainers of dumpstate told me
when I made changes to it back ~2016 or so: always exec
stuff, never link it.
Well, there have been quite a few divergences from this,
and it's come up in a couple of reviews I've seen here.
So, I've added notes on this into the readme and comments
on some of the worst offender. For instance, we could
have probably avoided a lot of this async/exec logic
for these custom cases if we used the standard run
command with timeout stuff.
Bug: N/A
Test: N/A
Change-Id: I4c33f3042ca16d4433082b8d78ebdd29cb75f0b3
diff --git a/cmds/dumpstate/Android.bp b/cmds/dumpstate/Android.bp
index be96306..1397ae6 100644
--- a/cmds/dumpstate/Android.bp
+++ b/cmds/dumpstate/Android.bp
@@ -83,14 +83,16 @@
"aconfig_lib_cc_static_link.defaults",
"dumpstate_cflag_defaults",
],
+ // See README.md: "Dumpstate philosophy: exec not link"
+ // Do not add things here - keep dumpstate as simple as possible and exec where possible.
shared_libs: [
"android.hardware.dumpstate@1.0",
"android.hardware.dumpstate@1.1",
"android.hardware.dumpstate-V1-ndk",
"libziparchive",
"libbase",
- "libbinder",
- "libbinder_ndk",
+ "libbinder", // BAD: dumpstate should not link code directly, should only exec binaries
+ "libbinder_ndk", // BAD: dumpstate should not link code directly, should only exec binaries
"libcrypto",
"libcutils",
"libdebuggerd_client",
@@ -98,11 +100,11 @@
"libdumpstateutil",
"libdumputils",
"libhardware_legacy",
- "libhidlbase",
+ "libhidlbase", // BAD: dumpstate should not link code directly, should only exec binaries
"liblog",
"libutils",
- "libvintf",
- "libbinderdebug",
+ "libvintf", // BAD: dumpstate should not link code directly, should only exec binaries
+ "libbinderdebug", // BAD: dumpstate should not link code directly, should only exec binaries
"packagemanager_aidl-cpp",
"server_configurable_flags",
"device_policy_aconfig_flags_c_lib",
diff --git a/cmds/dumpstate/README.md b/cmds/dumpstate/README.md
index 26dabbb..3ab971a 100644
--- a/cmds/dumpstate/README.md
+++ b/cmds/dumpstate/README.md
@@ -21,6 +21,18 @@
mmm -j frameworks/native/cmds/dumpstate device/acme/secret_device/dumpstate/ hardware/interfaces/dumpstate
```
+## Dumpstate philosophy: exec not link
+
+Never link code directly into dumpstate. Dumpstate should execute many
+binaries and collect the results. In general, code should fail hard fail fast,
+but dumpstate is the last to solve many Android bugs. Oftentimes, failures
+in core Android infrastructure or tools are issues that cause problems in
+bugreport directly, so bugreport should not rely on these tools working.
+We want dumpstate to have as minimal of code loaded in process so that
+only that core subset needs to be bugfree for bugreport to work. Even if
+many pieces of Android break, that should not prevent dumpstate from
+working.
+
## To build, deploy, and take a bugreport
```
diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp
index bb0ffe6..c24bee1 100644
--- a/cmds/dumpstate/dumpstate.cpp
+++ b/cmds/dumpstate/dumpstate.cpp
@@ -128,6 +128,9 @@
using android::os::dumpstate::TaskQueue;
using android::os::dumpstate::WaitForTask;
+// BAD - See README.md: "Dumpstate philosophy: exec not link"
+// Do not add more complicated variables here, prefer to execute only. Don't link more code here.
+
// Keep in sync with
// frameworks/base/services/core/java/com/android/server/am/ActivityManagerService.java
static const int TRACE_DUMP_TIMEOUT_MS = 10000; // 10 seconds
@@ -1266,6 +1269,8 @@
}
}
+// BAD - See README.md: "Dumpstate philosophy: exec not link"
+// This should all be moved into a separate binary rather than have complex logic here.
static Dumpstate::RunStatus RunDumpsysTextByPriority(const std::string& title, int priority,
std::chrono::milliseconds timeout,
std::chrono::milliseconds service_timeout) {
@@ -1346,6 +1351,8 @@
service_timeout);
}
+// BAD - See README.md: "Dumpstate philosophy: exec not link"
+// This should all be moved into a separate binary rather than have complex logic here.
static Dumpstate::RunStatus RunDumpsysProto(const std::string& title, int priority,
std::chrono::milliseconds timeout,
std::chrono::milliseconds service_timeout) {
@@ -1427,6 +1434,8 @@
* Dumpstate can pick up later and output to the bugreport. Using STDOUT_FILENO
* if it's not running in the parallel task.
*/
+// BAD - See README.md: "Dumpstate philosophy: exec not link"
+// This should all be moved into a separate binary rather than have complex logic here.
static void DumpHals(int out_fd = STDOUT_FILENO) {
RunCommand("HARDWARE HALS", {"lshal", "--all", "--types=all"},
CommandOptions::WithTimeout(10).AsRootIfAvailable().Build(),
@@ -1483,6 +1492,9 @@
}
}
+// BAD - See README.md: "Dumpstate philosophy: exec not link"
+// This should all be moved into a separate binary rather than have complex logic here.
+//
// Dump all of the files that make up the vendor interface.
// See the files listed in dumpFileList() for the latest list of files.
static void DumpVintf() {
@@ -1512,6 +1524,8 @@
printf("------ EXTERNAL FRAGMENTATION INFO ------\n");
std::ifstream ifs("/proc/buddyinfo");
auto unusable_index_regex = std::regex{"Node\\s+([0-9]+),\\s+zone\\s+(\\S+)\\s+(.*)"};
+ // BAD - See README.md: "Dumpstate philosophy: exec not link"
+ // This should all be moved into a separate binary rather than have complex logic here.
for (std::string line; std::getline(ifs, line);) {
std::smatch match_results;
if (std::regex_match(line, match_results, unusable_index_regex)) {
@@ -2452,6 +2466,8 @@
return dumpstate_hal_aidl::IDumpstateDevice::DumpstateMode::DEFAULT;
}
+// BAD - See README.md: "Dumpstate philosophy: exec not link"
+// This should all be moved into a separate binary rather than have complex logic here.
static void DoDumpstateBoardHidl(
const sp<dumpstate_hal_hidl_1_0::IDumpstateDevice> dumpstate_hal_1_0,
const std::vector<::ndk::ScopedFileDescriptor>& dumpstate_fds,