Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1819573002:
[Battor] Add ability to dump trace log to file. (Closed)
|
Created:
4 years, 9 months ago by rnephew (Reviews Here) Modified:
4 years, 9 months ago CC:
alexandermont, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Battor] Add ability to dump trace log to file.
BUG=595506
Committed: https://crrev.com/5f3f2d11370565d8c92d941beb9f554911675e66
Cr-Commit-Position: refs/heads/master@{#383070}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Charlies review #
Total comments: 8
Patch Set 3 : review part 2: the re-reviewing #
Total comments: 10
Patch Set 4 : #Patch Set 5 : rebase #Messages
Total messages: 24 (8 generated)
rnephew@chromium.org changed reviewers: + charliea@chromium.org
https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... File tools/battor_agent/battor_agent_bin.cc (right): https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:74: " StopTracing\n" Locally already changed to read: " StopTracing <Optional file path>\n" https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:81: const std::string* mDumpPath = nullptr; I know this is named wrong, but not sure what is right. https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:131: Already deleted this locally.
Great work - thanks so much for doing this Randy! Just a few code suggestions, none of which should be too terribly difficult to implement https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... File tools/battor_agent/battor_agent_bin.cc (right): https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:74: " StopTracing\n" On 2016/03/18 22:21:27, rnephew1 wrote: > Locally already changed to read: > " StopTracing <Optional file path>\n" Acknowledged. https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:81: const std::string* mDumpPath = nullptr; On 2016/03/18 22:21:27, rnephew1 wrote: > I know this is named wrong, but not sure what is right. 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_; https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:81: const std::string* mDumpPath = nullptr; 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. https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:131: On 2016/03/18 22:21:27, rnephew1 wrote: > Already deleted this locally. Acknowledged. https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:133: base::StringTokenizer tokenizer(cmd, " "); 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). https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:150: nit: remove extra line https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:218: void StopTracing(const std::string&path) { 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. https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:234: std::ofstream traceStream; nit: variable names are underscore delimited (not camelcase) https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:235: traceStream.open(*mDumpPath); I think that the above two lines can be written as: std::ofstream traceStream(mDumpPath); https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:236: traceStream << trace; 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/
https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... File tools/battor_agent/battor_agent_bin.cc (right): https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:81: const std::string* mDumpPath = nullptr; On 2016/03/21 15:19:15, charliea wrote: > On 2016/03/18 22:21:27, rnephew1 wrote: > > I know this is named wrong, but not sure what is right. > > 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_; Done. https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:133: base::StringTokenizer tokenizer(cmd, " "); On 2016/03/21 15:19:16, charliea wrote: > 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). Done. https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:150: On 2016/03/21 15:19:15, charliea wrote: > nit: remove extra line Done. https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:218: void StopTracing(const std::string&path) { On 2016/03/21 15:19:16, charliea wrote: > 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. Done. https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:234: std::ofstream traceStream; On 2016/03/21 15:19:15, charliea wrote: > nit: variable names are underscore delimited (not camelcase) Done. https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:235: traceStream.open(*mDumpPath); On 2016/03/21 15:19:15, charliea wrote: > I think that the above two lines can be written as: > > std::ofstream traceStream(mDumpPath); Done. https://codereview.chromium.org/1819573002/diff/1/tools/battor_agent/battor_a... tools/battor_agent/battor_agent_bin.cc:236: traceStream << trace; On 2016/03/21 15:19:16, charliea wrote: > 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/ Done.
Getting close! Just a few more things left that I think could be improved. https://codereview.chromium.org/1819573002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_agent_bin.cc (right): https://codereview.chromium.org/1819573002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:74: " StopTracing <Optional file path>\n" nit: "Optional" -> "optional" to match <marker> below https://codereview.chromium.org/1819573002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:214: trace_output_file_ = &path; 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* https://codereview.chromium.org/1819573002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:228: if (trace_output_file_) { If we do the above, this will have to be changed to if (trace_output_file_.empty()) // Write to STDOUT else // Write to file https://codereview.chromium.org/1819573002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:310: const std::string* trace_output_file_ = nullptr; As noted above, I think it makes sense to change this to a boring old std::string
https://codereview.chromium.org/1819573002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_agent_bin.cc (right): https://codereview.chromium.org/1819573002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:74: " StopTracing <Optional file path>\n" On 2016/03/22 17:45:26, charliea wrote: > nit: "Optional" -> "optional" to match <marker> below Done. https://codereview.chromium.org/1819573002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:214: trace_output_file_ = &path; On 2016/03/22 17:45:26, charliea wrote: > 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* > Done. https://codereview.chromium.org/1819573002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:228: if (trace_output_file_) { On 2016/03/22 17:45:26, charliea wrote: > If we do the above, this will have to be changed to > > if (trace_output_file_.empty()) > // Write to STDOUT > else > // Write to file Done. https://codereview.chromium.org/1819573002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:310: const std::string* trace_output_file_ = nullptr; On 2016/03/22 17:45:26, charliea wrote: > As noted above, I think it makes sense to change this to a boring old > std::string Done.
alexandermont@chromium.org changed reviewers: + alexandermont@chromium.org
https://codereview.chromium.org/1819573002/diff/40001/tools/battor_agent/batt... File tools/battor_agent/battor_agent_bin.cc (right): https://codereview.chromium.org/1819573002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:97: std::vector<std::string> TokenizeCommand(std::string cmd) { The name of this function could be more general, like "TokenizeString" - it works on any string, not just a string representing a command. https://codereview.chromium.org/1819573002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:137: } else if (cmd.find("StopTracing") != std::string::npos) { This just tests if "StopTracing" is in the string somewhere. I think you want to test if "StopTracing" is at the beginning of the string. https://codereview.chromium.org/1819573002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:153: } else if (cmd.find("RecordClockSyncMarker") != std::string::npos) { Same here; this just tests if "RecordClockSyncMarker" is somewhere in the string, not necessarily at the beginning.
https://codereview.chromium.org/1819573002/diff/40001/tools/battor_agent/batt... File tools/battor_agent/battor_agent_bin.cc (right): https://codereview.chromium.org/1819573002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:97: std::vector<std::string> TokenizeCommand(std::string cmd) { On 2016/03/23 22:56:53, alexandermont wrote: > The name of this function could be more general, like "TokenizeString" - it > works on any string, not just a string representing a command. This will only ever be used to tokenize command inputs though, so it makes the use of it more easily understood; I'll let Charlie weigh in and do whatever popular consensus is. https://codereview.chromium.org/1819573002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:137: } else if (cmd.find("StopTracing") != std::string::npos) { On 2016/03/23 22:56:53, alexandermont wrote: > This just tests if "StopTracing" is in the string somewhere. I think you want to > test if "StopTracing" is at the beginning of the string. Later it does make sure the first word is STopTracing and if it is not issues a warning that it is an invalid command. https://codereview.chromium.org/1819573002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:153: } else if (cmd.find("RecordClockSyncMarker") != std::string::npos) { On 2016/03/23 22:56:53, alexandermont wrote: > Same here; this just tests if "RecordClockSyncMarker" is somewhere in the > string, not necessarily at the beginning. Same as above.
lgtm. Thanks so much for taking this on Randy! https://codereview.chromium.org/1819573002/diff/40001/tools/battor_agent/batt... File tools/battor_agent/battor_agent_bin.cc (right): https://codereview.chromium.org/1819573002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:97: std::vector<std::string> TokenizeCommand(std::string cmd) { On 2016/03/24 00:11:26, rnephew1 wrote: > On 2016/03/23 22:56:53, alexandermont wrote: > > The name of this function could be more general, like "TokenizeString" - it > > works on any string, not just a string representing a command. > > This will only ever be used to tokenize command inputs though, so it makes the > use of it more easily understood; I'll let Charlie weigh in and do whatever > popular consensus is. I don't have any strong opinion here. I'm fine with TokenizeString, although I think they're both clear. https://codereview.chromium.org/1819573002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:218: AwaitResult(); After AwaitResult(), I'd do: trace_output_file_ = std::string();
https://codereview.chromium.org/1819573002/diff/40001/tools/battor_agent/batt... File tools/battor_agent/battor_agent_bin.cc (right): https://codereview.chromium.org/1819573002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:97: std::vector<std::string> TokenizeCommand(std::string cmd) { On 2016/03/24 15:05:21, charliea wrote: > On 2016/03/24 00:11:26, rnephew1 wrote: > > On 2016/03/23 22:56:53, alexandermont wrote: > > > The name of this function could be more general, like "TokenizeString" - it > > > works on any string, not just a string representing a command. > > > > This will only ever be used to tokenize command inputs though, so it makes the > > use of it more easily understood; I'll let Charlie weigh in and do whatever > > popular consensus is. > > I don't have any strong opinion here. I'm fine with TokenizeString, although I > think they're both clear. Switched to String https://codereview.chromium.org/1819573002/diff/40001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:218: AwaitResult(); On 2016/03/24 15:05:20, charliea wrote: > After AwaitResult(), I'd do: > > trace_output_file_ = std::string(); Done.
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from charliea@chromium.org Link to the patchset: https://codereview.chromium.org/1819573002/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1819573002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1819573002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
Just a rebase issue, fixed and trying again.
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from charliea@chromium.org Link to the patchset: https://codereview.chromium.org/1819573002/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1819573002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1819573002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Battor] Add ability to dump trace log to file. BUG=595506 ========== to ========== [Battor] Add ability to dump trace log to file. BUG=595506 Committed: https://crrev.com/5f3f2d11370565d8c92d941beb9f554911675e66 Cr-Commit-Position: refs/heads/master@{#383070} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5f3f2d11370565d8c92d941beb9f554911675e66 Cr-Commit-Position: refs/heads/master@{#383070} |
