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

Author: Steven Moreland

cmds/dumpstate/Android.bp

@@ -83,26 +83,28 @@ cc_defaults {
         "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",
         "libdumpstateaidl",
         "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",

cmds/dumpstate/README.md

@@ -21,6 +21,18 @@ Example:
 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
 
 ```

cmds/dumpstate/dumpstate.cpp

@@ -128,6 +128,9 @@ using android::os::dumpstate::PropertiesHelper;
 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 @@ static void DumpKernelMemoryAllocations() {
     }
 }
 
+// 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 @@ static Dumpstate::RunStatus RunDumpsysTextNormalPriority(const std::string& titl
                                     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 @@ static Dumpstate::RunStatus RunDumpsysNormal() {
  * 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 @@ static void DumpHals(int out_fd = STDOUT_FILENO) {
     }
 }
 
+// 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 @@ static void DumpExternalFragmentationInfo() {
     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 @@ static dumpstate_hal_aidl::IDumpstateDevice::DumpstateMode GetDumpstateHalModeAi
     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,