[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..c66cec77bbbb5739683b8e8c01e98a793c592c0b 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"
@@ -77,6 +78,8 @@ const char kUsage[] =
     "  Help\n"
     "\n";
 
+const std::string* mDumpPath = nullptr;

[rnephew (Reviews Here), 2016/03/18 22:21:27]

I know this is named wrong, but not sure what is right.

[charliea (OOO until 10-5), 2016/03/21 15:19:15]

What do you think about the name "trace output file" instead of "dump path"?
Trace output file seems to give the exact description of what the variable
contains (with not too many extra characters), whereas a dump path might mean
different things to different people.

[charliea (OOO until 10-5), 2016/03/21 15:19:15]

This should go down with the other member variables at the end of the file
(probably under "agent_"). Also, the Chromium convention to indicate that
something is a member variable is to add a trailing underscore rather than a
leading "m". 

In other words, this should probably look something like:

  // The agent capable of asynchronously communicating with the BattOr.
  scoped_ptr<BattOrAgent> agent_;

  std::string trace_output_file_;

[rnephew (Reviews Here), 2016/03/21 16:40:27]

Done.

+
 void PrintSupportsExplicitClockSync() {
   std::cout << BattOrAgent::SupportsExplicitClockSync() << endl;
 }
@@ -125,9 +128,26 @@ class BattOrAgentBin : public BattOrAgent::Listener {
 
       if (cmd == "StartTracing") {
         StartTracing();
-      } else if (cmd == "StopTracing") {
-        StopTracing();
+

[rnephew (Reviews Here), 2016/03/18 22:21:27]

Already deleted this locally.

[charliea (OOO until 10-5), 2016/03/21 15:19:16]

Acknowledged.

+      } else if (cmd.find("StopTracing") != std::string::npos) {
+        base::StringTokenizer tokenizer(cmd, " ");

[charliea (OOO until 10-5), 2016/03/21 15:19:16]

Could you abstract the splitting of the command into tokens into a separate
function? It looks like we're now doing it in two places (here and in
RecordClockSyncMarker), and it'd be nice to avoid the code repetition.

You can these sorts of static helper functions in the anonymous namespace (after
ExitFromThreadStartFailure in this case).

[rnephew (Reviews Here), 2016/03/21 16:40:27]

Done.

+        std::vector<std::string> tokens;
+        while (tokenizer.GetNext())
+          tokens.push_back(tokenizer.token());
+
+        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;
+

[charliea (OOO until 10-5), 2016/03/21 15:19:15]

nit: remove extra line

[rnephew (Reviews Here), 2016/03/21 16:40:27]

Done.

       } else if (cmd == "SupportsExplicitClockSync") {
         PrintSupportsExplicitClockSync();
       } else if (cmd.find("RecordClockSyncMarker") != std::string::npos) {
@@ -195,6 +215,11 @@ class BattOrAgentBin : public BattOrAgent::Listener {
     done_.Signal();
   }
 
+  void StopTracing(const std::string&path) {

[charliea (OOO until 10-5), 2016/03/21 15:19:16]

nit: should be a space between & and path. Also, if we do end up changing the
name of dump_path, we should probably change this variable name also.

[rnephew (Reviews Here), 2016/03/21 16:40:27]

Done.

+    mDumpPath = &path;
+    StopTracing();
+  }
+
   void StopTracing() {
     io_thread_.task_runner()->PostTask(
         FROM_HERE,
@@ -205,7 +230,14 @@ class BattOrAgentBin : public BattOrAgent::Listener {
   void OnStopTracingComplete(const std::string& trace,
                              BattOrError error) override {
     if (error == BATTOR_ERROR_NONE) {
-      std::cout << trace;
+      if (mDumpPath) {
+        std::ofstream traceStream;

[charliea (OOO until 10-5), 2016/03/21 15:19:15]

nit: variable names are underscore delimited (not camelcase)

[rnephew (Reviews Here), 2016/03/21 16:40:27]

Done.

+        traceStream.open(*mDumpPath);

[charliea (OOO until 10-5), 2016/03/21 15:19:15]

I think that the above two lines can be written as:

std::ofstream traceStream(mDumpPath);

[rnephew (Reviews Here), 2016/03/21 16:40:27]

Done.

+        traceStream << trace;

[charliea (OOO until 10-5), 2016/03/21 15:19:16]

Just about any time that you're opening and writing to a file, you should check
to make sure that the file opening was successful before moving on to writing. 

In this case, you can do that with:

if (!traceStream.is_open()) {
  // Print some error message.
  exit(1);
}

// Continue normally

Good resource: http://www.cplusplus.com/doc/tutorial/files/

[rnephew (Reviews Here), 2016/03/21 16:40:27]

Done.

+        traceStream.close();
+      } else {
+        std::cout << trace;
+      }
       std::cout << "Done." << endl;
     } else {
       HandleError(error);