Snap for 4696032 from 6ebf8824e2eda039168cff05684590e5b3a98bff to pi-release

Change-Id: I96bb4f2904843366bc736d873f78e415494ac36e

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