[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"

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

Locally already changed to read:
"  StopTracing <Optional file path>\n"

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

Acknowledged.

     "  SupportsExplicitClockSync\n"
     "  RecordClockSyncMarker <marker>\n"
     "  Exit\n"
     "  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;
 }
 
 // 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);
 }
@@ skipping 28 matching lines @@
     }
 
     SetUp(path);
 
     std::string cmd;
     for (;;) {
       std::getline(std::cin, cmd);
 
       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) {
         base::StringTokenizer tokenizer(cmd, " ");
 
         std::vector<std::string> tokens;
         while (tokenizer.GetNext())
           tokens.push_back(tokenizer.token());
 
         if (tokens.size() != 2 || tokens[0] != "RecordClockSyncMarker") {
@@ skipping 47 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) {

[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,
         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 (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);
     }
 
     done_.Signal();
   }
 
   void RecordClockSyncMarker(const std::string& marker) {
     io_thread_.task_runner()->PostTask(
@@ skipping 60 matching lines @@
 };
 
 }  // 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);
 }