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

tools/battor_agent/battor_agent_bin.cc

Index: tools/battor_agent/battor_agent_bin.cc
diff --git a/tools/battor_agent/battor_agent_bin.cc b/tools/battor_agent/battor_agent_bin.cc
index 1c00b470169916a9a2d6d7b3a01f3d7d2abc99cd..3a416a2534cefcffffe3360d8ee571db68d85535 100644
--- a/tools/battor_agent/battor_agent_bin.cc
+++ b/tools/battor_agent/battor_agent_bin.cc
@@ -33,6 +33,7 @@
 
 #include <stdint.h>
 
+#include <fstream>
 #include <iostream>
 
 #include "base/at_exit.h"
@@ -70,7 +71,7 @@ const char kUsage[] =
     "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"
@@ -93,6 +94,14 @@ 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
@@ -125,18 +134,24 @@ class BattOrAgentBin : public BattOrAgent::Listener {
 
       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;
@@ -195,6 +210,11 @@ class BattOrAgentBin : public BattOrAgent::Listener {
     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,
@@ -205,7 +225,17 @@ class BattOrAgentBin : public BattOrAgent::Listener {
   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);
@@ -276,6 +306,9 @@ class BattOrAgentBin : public BattOrAgent::Listener {
 
   // 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