Perfprofd: Use ConfigReader for shellCommand startProfiling

Use the ConfigReader for parsing arguments to startProfiling, so
that new arguments can be transparently picked up. This diverges
it from the AIDL interface, but is more functional and maintainable.

Also adjust some bounds.

(cherry picked from commit f73ddfa2db064d284ee9fa9821595a597d5ff2cb)

Bug: 73175642
Test: mmma system/extras/perfprofd
Test: perfprofd_test
Test: manual: adb shell cmd perfprofd startProfiling main_loop_iterations=2 compress=1 stack_profile=0 sampling_period=10000 sample_duration=10 collection_interval=1
Test: manual: adb shell cmd perfprofd startProfiling main_loop_iterations=2 compress=1 stack_profile=0 sampling_period=10000 sample_duration=10 collection_interval=1 bla
Merged-In: I7c437b43634aab70ea9463161fff6de303e427c8
Change-Id: I7c437b43634aab70ea9463161fff6de303e427c8

5 files changed, 94 insertions, 55 deletions

diff --git a/perfprofd/binder_interface/perfprofd_binder.cc b/perfprofd/binder_interface/perfprofd_binder.cc
index 87e0c5f6..8fc14828 100644
--- a/perfprofd/binder_interface/perfprofd_binder.cc
+++ b/perfprofd/binder_interface/perfprofd_binder.cc
@@ -48,6 +48,7 @@
 #include "perfprofd_record.pb.h"
 
 #include "config.h"
+#include "configreader.h"
 #include "dropbox.h"
 #include "perfprofdcore.h"
 #include "perfprofd_io.h"
@@ -115,8 +116,8 @@ class PerfProfdNativeService : public BinderService<PerfProfdNativeService>,
   status_t dump(int fd, const Vector<String16> &args) override;
 
   Status startProfiling(int32_t profilingDuration,
-                                int32_t profilingInterval,
-                                int32_t iterations) override;
+                        int32_t profilingInterval,
+                        int32_t iterations) override;
   Status startProfilingProtobuf(const std::vector<uint8_t>& config_proto) override;
 
   Status stopProfiling() override;
@@ -331,7 +332,7 @@ Status PerfProfdNativeService::stopProfiling() {
 
 status_t PerfProfdNativeService::shellCommand(int in,
                                               int out,
-                                              int err,
+                                              int err_fd,
                                               Vector<String16>& args) {
   if (android::base::kEnableDChecks) {
     LOG(VERBOSE) << "Perfprofd::shellCommand";
@@ -341,23 +342,31 @@ status_t PerfProfdNativeService::shellCommand(int in,
     }
   }
 
+  auto err_str = std::fstream(base::StringPrintf("/proc/self/fd/%d", err_fd));
+
   if (args.size() >= 1) {
     if (args[0] == String16("dump")) {
       dump(out, args);
       return OK;
     } else if (args[0] == String16("startProfiling")) {
-      if (args.size() < 4) {
-        return BAD_VALUE;
+      ConfigReader reader;
+      for (size_t i = 1; i < args.size(); ++i) {
+        if (!reader.Read(String8(args[i]).string(), /* fail_on_error */ true)) {
+          err_str << base::StringPrintf("Could not parse %s", String8(args[i]).string())
+                  << std::endl;
+          return BAD_VALUE;
+        }
       }
-      // TODO: handle invalid strings.
-      int32_t duration = strtol(String8(args[1]).string(), nullptr, 0);
-      int32_t interval = strtol(String8(args[2]).string(), nullptr, 0);
-      int32_t iterations = strtol(String8(args[3]).string(), nullptr, 0);
-      Status status = startProfiling(duration, interval, iterations);
+      auto config_fn = [&](BinderConfig& config) {
+        config = BinderConfig();  // Reset to a default config.
+        reader.FillConfig(&config);
+      };
+      Status status = StartProfiling(config_fn);
       if (status.isOk()) {
         return OK;
       } else {
-        return status.serviceSpecificErrorCode();
+        err_str << status.toString8() << std::endl;
+        return UNKNOWN_ERROR;
       }
     } else if (args[0] == String16("startProfilingProto")) {
       if (args.size() < 2) {
@@ -370,20 +379,23 @@ status_t PerfProfdNativeService::shellCommand(int in,
         // TODO: Implement reading from disk?
       }
       if (fd < 0) {
+        err_str << "Bad file descriptor " << args[1] << std::endl;
         return BAD_VALUE;
       }
       binder::Status status = StartProfilingProtobufFd(fd);
       if (status.isOk()) {
         return OK;
       } else {
-        return status.serviceSpecificErrorCode();
+        err_str << status.toString8() << std::endl;
+        return UNKNOWN_ERROR;
       }
     } else if (args[0] == String16("stopProfiling")) {
       Status status = stopProfiling();
       if (status.isOk()) {
         return OK;
       } else {
-        return status.serviceSpecificErrorCode();
+        err_str << status.toString8() << std::endl;
+        return UNKNOWN_ERROR;
       }
     }
   }
diff --git a/perfprofd/configreader.cc b/perfprofd/configreader.cc
index d3396b33..ac78f276 100644
--- a/perfprofd/configreader.cc
+++ b/perfprofd/configreader.cc
@@ -60,79 +60,86 @@ void ConfigReader::setConfigFilePath(const char *path)
 //
 void ConfigReader::addDefaultEntries()
 {
+  struct DummyConfig : public Config {
+    void Sleep(size_t seconds) override {}
+    bool IsProfilingEnabled() const override { return false; }
+  };
+  DummyConfig config;
+
   // Average number of seconds between perf profile collections (if
   // set to 100, then over time we want to see a perf profile
   // collected every 100 seconds). The actual time within the interval
   // for the collection is chosen randomly.
-  addUnsignedEntry("collection_interval", 14400, 100, UINT32_MAX);
+  addUnsignedEntry("collection_interval", config.collection_interval_in_s, 1, UINT32_MAX);
 
   // Use the specified fixed seed for random number generation (unit
   // testing)
-  addUnsignedEntry("use_fixed_seed", 0, 0, UINT32_MAX);
+  addUnsignedEntry("use_fixed_seed", config.use_fixed_seed, 0, UINT32_MAX);
 
   // For testing purposes, number of times to iterate through main
   // loop.  Value of zero indicates that we should loop forever.
-  addUnsignedEntry("main_loop_iterations", 0, 0, UINT32_MAX);
+  addUnsignedEntry("main_loop_iterations", config.main_loop_iterations, 0, UINT32_MAX);
 
   // Destination directory (where to write profiles). This location
   // chosen since it is accessible to the uploader service.
-  addStringEntry("destination_directory", "/data/misc/perfprofd");
+  addStringEntry("destination_directory", config.destination_directory.c_str());
 
   // Config directory (where to read configs).
-  addStringEntry("config_directory", "/data/data/com.google.android.gms/files");
+  addStringEntry("config_directory", config.config_directory.c_str());
 
   // Full path to 'perf' executable.
-  addStringEntry("perf_path", "/system/xbin/simpleperf");
+  addStringEntry("perf_path", config.perf_path.c_str());
 
-  // Desired sampling period (passed to perf -c option). Small
-  // sampling periods can perturb the collected profiles, so enforce
-  // min/max.
-  addUnsignedEntry("sampling_period", 500000, 5000, UINT32_MAX);
+  // Desired sampling period (passed to perf -c option).
+  addUnsignedEntry("sampling_period", config.sampling_period, 1, UINT32_MAX);
 
   // Length of time to collect samples (number of seconds for 'perf
   // record -a' run).
-  addUnsignedEntry("sample_duration", 3, 2, 600);
+  addUnsignedEntry("sample_duration", config.sample_duration_in_s, 1, 600);
 
   // If this parameter is non-zero it will cause perfprofd to
   // exit immediately if the build type is not userdebug or eng.
   // Currently defaults to 1 (true).
-  addUnsignedEntry("only_debug_build", 1, 0, 1);
+  addUnsignedEntry("only_debug_build", config.only_debug_build ? 1 : 0, 0, 1);
 
   // If the "mpdecision" service is running at the point we are ready
   // to kick off a profiling run, then temporarily disable the service
   // and hard-wire all cores on prior to the collection run, provided
   // that the duration of the recording is less than or equal to the value of
   // 'hardwire_cpus_max_duration'.
-  addUnsignedEntry("hardwire_cpus", 1, 0, 1);
-  addUnsignedEntry("hardwire_cpus_max_duration", 5, 1, UINT32_MAX);
+  addUnsignedEntry("hardwire_cpus", config.hardwire_cpus, 0, 1);
+  addUnsignedEntry("hardwire_cpus_max_duration",
+                   config.hardwire_cpus_max_duration_in_s,
+                   1,
+                   UINT32_MAX);
 
   // Maximum number of unprocessed profiles we can accumulate in the
   // destination directory. Once we reach this limit, we continue
   // to collect, but we just overwrite the most recent profile.
-  addUnsignedEntry("max_unprocessed_profiles", 10, 1, UINT32_MAX);
+  addUnsignedEntry("max_unprocessed_profiles", config.max_unprocessed_profiles, 1, UINT32_MAX);
 
   // If set to 1, pass the -g option when invoking 'perf' (requests
   // stack traces as opposed to flat profile).
-  addUnsignedEntry("stack_profile", 0, 0, 1);
+  addUnsignedEntry("stack_profile", config.stack_profile ? 1 : 0, 0, 1);
 
   // For unit testing only: if set to 1, emit info messages on config
   // file parsing.
-  addUnsignedEntry("trace_config_read", 0, 0, 1);
+  addUnsignedEntry("trace_config_read", config.trace_config_read ? 1 : 0, 0, 1);
 
   // Control collection of various additional profile tags
-  addUnsignedEntry("collect_cpu_utilization", 1, 0, 1);
-  addUnsignedEntry("collect_charging_state", 1, 0, 1);
-  addUnsignedEntry("collect_booting", 1, 0, 1);
-  addUnsignedEntry("collect_camera_active", 0, 0, 1);
+  addUnsignedEntry("collect_cpu_utilization", config.collect_cpu_utilization ? 1 : 0, 0, 1);
+  addUnsignedEntry("collect_charging_state", config.collect_charging_state ? 1 : 0, 0, 1);
+  addUnsignedEntry("collect_booting", config.collect_booting ? 1 : 0, 0, 1);
+  addUnsignedEntry("collect_camera_active", config.collect_camera_active ? 1 : 0, 0, 1);
 
   // If true, use an ELF symbolizer to on-device symbolize.
-  addUnsignedEntry("use_elf_symbolizer", 1, 0, 1);
+  addUnsignedEntry("use_elf_symbolizer", config.use_elf_symbolizer ? 1 : 0, 0, 1);
 
   // If true, use libz to compress the output proto.
-  addUnsignedEntry("compress", 0, 0, 1);
+  addUnsignedEntry("compress", config.compress ? 1 : 0, 0, 1);
 
-  // If true, send the proto to dropbox instead to a file.
-  addUnsignedEntry("dropbox", 0, 0, 1);
+  // If true, send the proto to dropbox instead of to a file.
+  addUnsignedEntry("dropbox", config.send_to_dropbox ? 1 : 0, 0, 1);
 }
 
 void ConfigReader::addUnsignedEntry(const char *key,
@@ -205,7 +212,7 @@ void ConfigReader::overrideUnsignedEntry(const char *key, unsigned new_value)
 // warnings or errors to the system logs if the line can't be
 // interpreted properly.
 //
-void ConfigReader::parseLine(const char *key,
+bool ConfigReader::parseLine(const char *key,
                              const char *value,
                              unsigned linecount)
 {
@@ -217,6 +224,7 @@ void ConfigReader::parseLine(const char *key,
     unsigned uvalue = 0;
     if (isdigit(value[0]) == 0 || sscanf(value, "%u", &uvalue) != 1) {
       LOG(WARNING) << "line " << linecount << ": malformed unsigned value (ignored)";
+      return false;
     } else {
       values vals;
       auto iit = u_info.find(key);
@@ -227,6 +235,7 @@ void ConfigReader::parseLine(const char *key,
                      << "specified value " << uvalue << " for '" << key << "' "
                      << "outside permitted range [" << vals.minv << " " << vals.maxv
                      << "] (ignored)";
+        return false;
       } else {
         if (trace_config_read) {
           LOG(INFO) << "option " << key << " set to " << uvalue;
@@ -235,7 +244,7 @@ void ConfigReader::parseLine(const char *key,
       }
     }
     trace_config_read = (getUnsignedValue("trace_config_read") != 0);
-    return;
+    return true;
   }
 
   auto sit = s_entries.find(key);
@@ -244,10 +253,11 @@ void ConfigReader::parseLine(const char *key,
       LOG(INFO) << "option " << key << " set to " << value;
     }
     sit->second = std::string(value);
-    return;
+    return true;
   }
 
   LOG(WARNING) << "line " << linecount << ": unknown option '" << key << "' ignored";
+  return false;
 }
 
 static bool isblank(const std::string &line)
@@ -256,14 +266,19 @@ static bool isblank(const std::string &line)
   return std::find_if(line.begin(), line.end(), non_space) == line.end();
 }
 
+
+
 bool ConfigReader::readFile()
 {
   std::string contents;
   if (! android::base::ReadFileToString(config_file_path, &contents)) {
     return false;
   }
+  return Read(contents, /* fail_on_error */ false);
+}
 
-  std::stringstream ss(contents);
+bool ConfigReader::Read(const std::string& content, bool fail_on_error) {
+  std::stringstream ss(content);
   std::string line;
   for (unsigned linecount = 1;
        std::getline(ss,line,'\n');
@@ -284,13 +299,19 @@ bool ConfigReader::readFile()
     auto efound = line.find('=');
     if (efound == std::string::npos) {
       LOG(WARNING) << "line " << linecount << ": line malformed (no '=' found)";
+      if (fail_on_error) {
+        return false;
+      }
       continue;
     }
 
     std::string key(line.substr(0, efound));
     std::string value(line.substr(efound+1, std::string::npos));
 
-    parseLine(key.c_str(), value.c_str(), linecount);
+    bool parse_success = parseLine(key.c_str(), value.c_str(), linecount);
+    if (fail_on_error && !parse_success) {
+      return false;
+    }
   }
 
   return true;
diff --git a/perfprofd/configreader.h b/perfprofd/configreader.h
index 432a12df..7b6bcf30 100644
--- a/perfprofd/configreader.h
+++ b/perfprofd/configreader.h
@@ -44,6 +44,8 @@ class ConfigReader {
   // returns true for successful read, false if conf file cannot be opened.
   bool readFile();
 
+  bool Read(const std::string& data, bool fail_on_error);
+
   // set/get path to config file
   static void setConfigFilePath(const char *path);
   static const char *getConfigFilePath();
@@ -60,7 +62,7 @@ class ConfigReader {
                         unsigned max_value);
   void addStringEntry(const char *key, const char *default_value);
   void addDefaultEntries();
-  void parseLine(const char *key, const char *value, unsigned linecount);
+  bool parseLine(const char *key, const char *value, unsigned linecount);
 
   typedef struct { unsigned minv, maxv; } values;
   std::map<std::string, values> u_info;
diff --git a/perfprofd/perfprofdcore.cc b/perfprofd/perfprofdcore.cc
index a5b4b480..69508ad6 100644
--- a/perfprofd/perfprofdcore.cc
+++ b/perfprofd/perfprofdcore.cc
@@ -481,7 +481,7 @@ static PROFILE_RESULT invoke_perf(Config& config,
     }
 
     // marshall arguments
-    constexpr unsigned max_args = 14;
+    constexpr unsigned max_args = 15;
     const char *argv[max_args];
     unsigned slot = 0;
     argv[slot++] = perf_path.c_str();
@@ -510,11 +510,12 @@ static PROFILE_RESULT invoke_perf(Config& config,
       argv[slot++] = pid_str.c_str();
     }
 
-    // no need for kernel symbols
+    // no need for kernel or other symbols
     argv[slot++] = "--no-dump-kernel-symbols";
+    argv[slot++] = "--no-dump-symbols";
 
     // sleep <duration>
-    argv[slot++] = "/system/bin/sleep";
+    argv[slot++] = "--duration";
     std::string d_str = android::base::StringPrintf("%u", duration);
     argv[slot++] = d_str.c_str();
 
@@ -761,9 +762,12 @@ static void ProfilingLoopImpl(ConfigFn config, UpdateFn update, HandlerFn handle
     // run perf
     unsigned sleep_before_collect = 0;
     unsigned sleep_after_collect = 0;
-    determine_before_after(sleep_before_collect, sleep_after_collect,
+    determine_before_after(sleep_before_collect,
+                           sleep_after_collect,
                            config()->collection_interval_in_s);
-    config()->Sleep(sleep_before_collect);
+    if (sleep_before_collect > 0) {
+      config()->Sleep(sleep_before_collect);
+    }
 
     if (config()->ShouldStopProfiling()) {
       return;
@@ -797,7 +801,9 @@ static void ProfilingLoopImpl(ConfigFn config, UpdateFn update, HandlerFn handle
       return;
     }
 
-    config()->Sleep(sleep_after_collect);
+    if (sleep_after_collect > 0) {
+      config()->Sleep(sleep_after_collect);
+    }
     iterations += 1;
   }
 }
diff --git a/perfprofd/tests/perfprofd_test.cc b/perfprofd/tests/perfprofd_test.cc
index 79f8ea64..1f5ae82b 100644
--- a/perfprofd/tests/perfprofd_test.cc
+++ b/perfprofd/tests/perfprofd_test.cc
@@ -614,7 +614,6 @@ TEST_F(PerfProfdTest, ConfigFileParsing)
   // assorted bad syntax
   runner.addToConfig("collection_interval=0");
   runner.addToConfig("collection_interval=-1");
-  runner.addToConfig("collection_interval=2");
   runner.addToConfig("nonexistent_key=something");
   runner.addToConfig("no_equals_stmt");
 
@@ -626,11 +625,10 @@ TEST_F(PerfProfdTest, ConfigFileParsing)
 
   // Verify log contents
   const std::string expected = RAW_RESULT(
-      W: line 6: specified value 0 for 'collection_interval' outside permitted range [100 4294967295] (ignored)
+      W: line 6: specified value 0 for 'collection_interval' outside permitted range [1 4294967295] (ignored)
       W: line 7: malformed unsigned value (ignored)
-      W: line 8: specified value 2 for 'collection_interval' outside permitted range [100 4294967295] (ignored)
-      W: line 9: unknown option 'nonexistent_key' ignored
-      W: line 10: line malformed (no '=' found)
+      W: line 8: unknown option 'nonexistent_key' ignored
+      W: line 9: line malformed (no '=' found)
                                           );
 
   // check to make sure log excerpt matches