[Battor] Add ability to dump trace log to file.

tools/battor_agent/battor_agent_bin.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // This file provides a thin binary wrapper around the BattOr Agent
 // library. This binary wrapper provides a means for non-C++ tracing
 // controllers, such as Telemetry and Android Systrace, to issue high-level
 // tracing commands to the BattOr through an interactive shell.
 //
 // Example usage of how an external trace controller might use this binary:
@@ skipping 15 matching lines @@
 //    sync end timestamp and sends the subprocess the StopTracing message via
 //    STDIN
 // 9) PowerTracingAgent continues to read trace output lines from STDOUT until
 //    the binary exits with an exit code of 1 (indicating failure) or the
 //    'Done.' line is printed to STDOUT, signaling the last line of the trace
 // 10) PowerTracingAgent returns the battery trace to the Telemetry trace
 //     controller
 
 #include <stdint.h>
 
+#include <fstream>
 #include <iostream>
 
 #include "base/at_exit.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/strings/string_tokenizer.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 17 matching lines @@
     "Start the battor_agent shell with:\n"
     "\n"
     "  battor_agent <switches>\n"
     "\n"
     "Switches: \n"
     "  --battor-path=<path> Uses the specified BattOr path.\n"
     "\n"
     "Once in the shell, you can issue the following commands:\n"
     "\n"
     "  StartTracing\n"
-    "  StopTracing\n"
+    "  StopTracing <Optional file path>\n"

[charliea (OOO until 10-5), 2016/03/22 17:45:26]

nit: "Optional" -> "optional" to match <marker> below

[rnephew (Reviews Here), 2016/03/22 18:05:46]

Done.

     "  SupportsExplicitClockSync\n"
     "  RecordClockSyncMarker <marker>\n"
     "  Exit\n"
     "  Help\n"
     "\n";
 
 void PrintSupportsExplicitClockSync() {
   std::cout << BattOrAgent::SupportsExplicitClockSync() << endl;
 }
 
 // Logs the error and exits with an error code.
 void HandleError(battor::BattOrError error) {
   if (error != BATTOR_ERROR_NONE)
     LOG(FATAL) << "Fatal error when communicating with the BattOr: "
                << BattOrErrorToString(error);
 }
 
 // Prints an error message and exits due to a required thread failing to start.
 void ExitFromThreadStartFailure(const std::string& thread_name) {
   LOG(FATAL) << "Failed to start " << thread_name;
 }
 
+std::vector<std::string> TokenizeCommand(std::string cmd) {
+  base::StringTokenizer tokenizer(cmd, " ");
+  std::vector<std::string> tokens;
+  while (tokenizer.GetNext())
+    tokens.push_back(tokenizer.token());
+  return tokens;
+}
+
 }  // namespace
 
 // Wrapper class containing all state necessary for an independent binary to
 // use a BattOrAgent to communicate with a BattOr.
 class BattOrAgentBin : public BattOrAgent::Listener {
  public:
   BattOrAgentBin()
       : done_(false, false),
         io_thread_(kIoThreadName),
         file_thread_(kFileThreadName),
@@ skipping 12 matching lines @@
     }
 
     SetUp(path);
 
     std::string cmd;
     for (;;) {
       std::getline(std::cin, cmd);
 
       if (cmd == "StartTracing") {
         StartTracing();
-      } else if (cmd == "StopTracing") {
-        StopTracing();
+      } else if (cmd.find("StopTracing") != std::string::npos) {
+        std::vector<std::string> tokens = TokenizeCommand(cmd);
+        if (tokens.size() == 1 && tokens[0] == "StopTracing") {
+          // No path given.
+          StopTracing();
+        } else if (tokens.size() == 2 && tokens[0] == "StopTracing") {
+          // Path given.
+          StopTracing(tokens[1]);
+        } else {
+          std::cout << "Invalid StopTracing command." << endl;
+          std::cout << kUsage << endl;
+          continue;
+        }
         break;
       } else if (cmd == "SupportsExplicitClockSync") {
         PrintSupportsExplicitClockSync();
       } else if (cmd.find("RecordClockSyncMarker") != std::string::npos) {
-        base::StringTokenizer tokenizer(cmd, " ");
-
-        std::vector<std::string> tokens;
-        while (tokenizer.GetNext())
-          tokens.push_back(tokenizer.token());
-
+        std::vector<std::string> tokens = TokenizeCommand(cmd);
         if (tokens.size() != 2 || tokens[0] != "RecordClockSyncMarker") {
           std::cout << "Invalid RecordClockSyncMarker command." << endl;
           std::cout << kUsage << endl;
           continue;
         }
 
         RecordClockSyncMarker(tokens[1]);
       } else if (cmd == "Exit") {
         break;
       } else {
@@ skipping 38 matching lines @@
 
   void OnStartTracingComplete(BattOrError error) override {
     if (error == BATTOR_ERROR_NONE)
       std::cout << "Done." << endl;
     else
       HandleError(error);
 
     done_.Signal();
   }
 
+  void StopTracing(const std::string& path) {
+    trace_output_file_ = &path;

[charliea (OOO until 10-5), 2016/03/22 17:45:26]

This is a scary line - you're taking the address of a const reference that was
passed in and storing that address as a local variable. This means that we're
assuming that whatever variable was passed in will stay in scope for longer as
the class member is used for. In this particular case, I think that we DO know
this for sure because we're calling AwaitResult() before returning, but it still
takes quite a bit of reasoning to reach that conclusion (it's not painfully
obvious, which is definitely what we want the code to be).

I think that we can eliminate the two versions of StopTracing using a default
parameter and consolidate them into:

void StopTracing(const std::string& path = "") {
  trace_output_file_ = path;
  io_thread_.task_runner()->PostTask(
  ... and so on
}

In other words, have trace_output_file_ be a std::string instead of a const
std::string*

[rnephew (Reviews Here), 2016/03/22 18:05:46]

Done.

+    StopTracing();
+  }
+
   void StopTracing() {
     io_thread_.task_runner()->PostTask(
         FROM_HERE,
         base::Bind(&BattOrAgent::StopTracing, base::Unretained(agent_.get())));
     AwaitResult();
   }
 
   void OnStopTracingComplete(const std::string& trace,
                              BattOrError error) override {
     if (error == BATTOR_ERROR_NONE) {
-      std::cout << trace;
+      if (trace_output_file_) {

[charliea (OOO until 10-5), 2016/03/22 17:45:26]

If we do the above, this will have to be changed to 

if (trace_output_file_.empty())
  // Write to STDOUT
else
  // Write to file

[rnephew (Reviews Here), 2016/03/22 18:05:46]

Done.

+        std::ofstream trace_stream(*trace_output_file_);
+        if (!trace_stream.is_open()) {
+          std::cout << "Tracing output file could not be opened." << endl;
+          exit(1);
+        }
+        trace_stream << trace;
+        trace_stream.close();
+      } else {
+        std::cout << trace;
+      }
       std::cout << "Done." << endl;
     } else {
       HandleError(error);
     }
 
     done_.Signal();
   }
 
   void RecordClockSyncMarker(const std::string& marker) {
     io_thread_.task_runner()->PostTask(
@@ skipping 50 matching lines @@
   // Event signaled when an async task has finished executing.
   base::WaitableEvent done_;
 
   // Threads needed for serial communication.
   base::Thread io_thread_;
   base::Thread file_thread_;
   base::Thread ui_thread_;
 
   // The agent capable of asynchronously communicating with the BattOr.
   scoped_ptr<BattOrAgent> agent_;
+
+  const std::string* trace_output_file_ = nullptr;

[charliea (OOO until 10-5), 2016/03/22 17:45:26]

As noted above, I think it makes sense to change this to a boring old
std::string

[rnephew (Reviews Here), 2016/03/22 18:05:46]

Done.

+
 };
 
 }  // namespace battor
 
 int main(int argc, char* argv[]) {
   base::AtExitManager exit_manager;
   base::CommandLine::Init(argc, argv);
   battor::BattOrAgentBin bin;
   return bin.Run(argc, argv);
 }