Tool added to extract network traffic annotations.

tools/traffic_annotation/auditor/traffic_annotation_auditor.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.

[msramek, 2017/02/21 15:57:54]

nit: 2017

[Ramin Halavati, 2017/02/23 12:09:50]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// See README.txt for information and build instructions.

[msramek, 2017/02/21 15:57:54]

Also here :)

[Ramin Halavati, 2017/02/23 12:09:50]

Done.

+
+#include <cstdio>
+#include <cstdlib>
+#include <fstream>
+#include <map>
+#include <string>
+
+#include "base/command_line.h"
+#include "base/files/file_enumerator.h"
+#include "base/files/file_path.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/strings/stringprintf.h"
+#include "net/traffic_annotation/network_traffic_annotation.h"
+#include "third_party/protobuf/src/google/protobuf/text_format.h"
+
+#include "tools/traffic_annotation/traffic_annotation.pb.h"
+

[msramek, 2017/02/21 15:57:53]

Please use an anonymous namespace for helper classes.

[Ramin Halavati, 2017/02/23 12:09:49]

Done.

+// Holds an instance of network traffic annotation

[msramek, 2017/02/21 15:57:54]

nit: Period at the end of the sentence.

[Ramin Halavati, 2017/02/23 12:09:49]

Done.

+struct Instance {

[msramek, 2017/02/21 15:57:54]

This is very generic. How about AnnotationInstance?

[Ramin Halavati, 2017/02/23 12:09:49]

Done.

+  Instance() : state(kOK) {}
+
+  traffic_annotation::NetworkTrafficAnnotation proto;

[msramek, 2017/02/21 15:57:54]

Please add documentation for public attributes.

[Ramin Halavati, 2017/02/23 12:09:50]

Done.

+  std::string unique_id;
+  std::string callee;
+  enum { kOK, kDuplicate, kUndefined } state;

[msramek, 2017/02/21 15:57:54]

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md


According to the style guide, kConstantNaming is only used for the "enum hack"
to define an int constant; however, MACRO_STYLE is used for actual enumerations.

States of an instance are an enumeration. I suggest one of the following:

enum State {
  STATE_OK,
  STATE_DUPLICATE,
  STATE_UNDEFINED
}

or

enum class State {
  OK,
  DUPLICATE,
  UNDEFINED
}

[Ramin Halavati, 2017/02/23 12:09:49]

Done.

+};
+
+// Runs clang tool, returns true if all OK.

[msramek, 2017/02/21 15:57:54]

nit: 'if successful' ?

[Ramin Halavati, 2017/02/23 12:09:50]

Done.

+bool RunClangTool(const base::FilePath& src_dir,
+                  const base::FilePath& build_dir,
+                  const base::FilePath& output_dir,
+                  const base::CommandLine::StringVector& path_filters) {
+#ifdef _WIN32
+  int result1 = system(
+      base::StringPrintf(
+          "python %s %s",
+          src_dir.AppendASCII("tools/clang/scripts/generate_win_compdb.py")
+              .MaybeAsASCII()
+              .c_str(),
+          build_dir.MaybeAsASCII().c_str())
+          .c_str());
+  if (result1) {
+    printf(" Executing generate_win_compdb returned code: %i.\n", result1);
+    return false;
+  }
+#endif
+  for (auto& path : path_filters) {
+#ifdef _WIN32
+    std::string command(
+        "python %s traffic_annotation_extractor %s %S --tool-args %s ");

[msramek, 2017/02/21 15:57:53]

Is the capital S intentional?

[Ramin Halavati, 2017/02/23 12:09:50]

Yes, it's to convert |path| from ASCII to unicode.

+#else
+    std::string command(
+        "%s traffic_annotation_extractor --generate-compdb %s %s --tool-args "
+        "%s");
+#endif
+    int result2 =

[msramek, 2017/02/21 15:57:53]

I would prefer more semantic names than |result1| and |result2|. Or actually we
can just reuse |result|.

[Ramin Halavati, 2017/02/23 12:09:49]

Done, variable is temporary and used just to avoid computing twice for error
reporting.

+        system(base::StringPrintf(
+                   command.c_str(),
+                   src_dir.AppendASCII("tools/clang/scripts/run_tool.py")
+                       .MaybeAsASCII()
+                       .c_str(),
+                   build_dir.MaybeAsASCII().c_str(), path.c_str(),
+                   output_dir.MaybeAsASCII().c_str())
+                   .c_str());
+    if (result2) {
+      printf(" Executing clang tool returned code: %i.\n", result2);
+      return false;
+    }
+  }
+  return true;
+}
+
+// Reads an extrated annotation file. The first 6 lines of the file include:

[msramek, 2017/02/21 15:57:54]

typo: extracted

[Ramin Halavati, 2017/02/23 12:09:50]

Done.

+//   0- File path.
+//   1- Name of the function that is called using annotation.
+//   2- Line number.
+//   3- Name of the function including this position.
+//   4- Error text associated with this annotation.
+//   5- Unique id of annotation.
+// The rest of the file is the protobuf text.
+bool ReadFile(const base::FilePath& file_path,
+              std::vector<std::string>* header_lines,

[msramek, 2017/02/21 15:57:54]

I assume that this is the first 6 lines and the below is the protobuf text.
Please make this obvious in the method doc, e.g.

"Reads an extracted annotation file and returns it in the output variables. The
file starts with six |header_lines| with the following meaning:"

[Ramin Halavati, 2017/02/23 12:09:49]

Done.

+              std::string* msg_text) {
+  std::ifstream input_file(file_path.value());

[msramek, 2017/02/21 15:57:54]

Should we use base::File instead?

[Ramin Halavati, 2017/02/23 12:09:49]

Done.

+  if (!input_file.is_open())
+    return false;
+
+  header_lines->clear();
+  msg_text->clear();
+
+  while (!input_file.eof()) {
+    std::string line;
+    getline(input_file, line);
+    if (header_lines->size() < 6)
+      header_lines->push_back(line);
+    else
+      *msg_text = *msg_text + "\n" + line;
+  }
+  input_file.close();
+  return true;
+}
+
+// Reads all extracted txt files form given input folder and populates instances

[msramek, 2017/02/21 15:57:54]

typo: from

[Ramin Halavati, 2017/02/23 12:09:49]

Done.

+// and errors.

[msramek, 2017/02/21 15:57:54]

What are errors in this case?

[Ramin Halavati, 2017/02/23 12:09:49]

|errors| vector added, comment expanded.

+void ReadExtractedFiles(const base::FilePath& folder_name,
+                        std::vector<Instance>* instances) {
+#ifdef _WIN32
+  base::FileEnumerator file_iter(folder_name, false,
+                                 base::FileEnumerator::FILES, L"*.txt");
+#else
+  base::FileEnumerator file_iter(folder_name, false,
+                                 base::FileEnumerator::FILES, "*.txt");
+#endif
+  while (!file_iter.Next().empty()) {
+    std::string file_name = file_iter.GetInfo().GetName().AsUTF8Unsafe();
+    printf("\tReading %s...\n", file_name.c_str());
+
+    std::vector<std::string> header_lines;
+    std::string msg_text;
+    if (!ReadFile(folder_name.Append(file_iter.GetInfo().GetName()),
+                  &header_lines, &msg_text)) {
+      fprintf(stderr, "Could not open file '%s'\n.", file_name.c_str());
+      continue;
+    }
+    if (header_lines.size() < 6) {
+      fprintf(stderr, "Header lines are not complete for file '%s'\n.",
+              file_name.c_str());
+      continue;
+    }
+    if (!header_lines[4].empty()) {
+      fprintf(stderr, "Error passed from extractor for file '%s': %s\n.",
+              file_name.c_str(), header_lines[4].c_str());
+      continue;
+    }
+
+    Instance new_instance;
+    if (header_lines[5] == "\"Undefined\"") {
+      new_instance.state = Instance::kUndefined;
+    } else if (!google::protobuf::TextFormat::ParseFromString(
+                   msg_text, (google::protobuf::Message*)&new_instance)) {
+      fprintf(stderr, "Could not parse protobuf for file '%s'\n.",
+              file_name.c_str());
+      continue;
+    }
+
+    // Add header data to new instance.
+    traffic_annotation::NetworkTrafficAnnotation_TrafficSource* src =
+        new_instance.proto.mutable_source();
+    src->set_file(header_lines[0]);
+    src->set_function(header_lines[1]);
+    src->set_line(atoi(header_lines[2].c_str()));

[msramek, 2017/02/21 15:57:53]

base::StringToInt

[Ramin Halavati, 2017/02/23 12:09:50]

Done.

+    new_instance.callee = header_lines[3];
+    new_instance.unique_id = header_lines[5];
+    instances->push_back(new_instance);
+  }
+  printf("%i annotation instance(s) read.\n", (int)instances->size());

[msramek, 2017/02/21 15:57:55]

You can use %ud instead of the cast.

[Ramin Halavati, 2017/02/23 12:09:49]

Done.

+}
+
+// Checks to see if unique ids are really unique and marks the ones which are
+// not.
+void MarkRepeatedUniqueIds(std::vector<Instance>* instances) {
+  std::map<std::string, Instance*> unique_ids;
+  for (auto& instance : *instances) {
+    if (instance.state == Instance::kOK) {
+      auto match = unique_ids.find(instance.unique_id);
+      if (match != unique_ids.end())
+        instance.state = match->second->state = Instance::kDuplicate;
+      else
+        unique_ids.insert(
+            std::make_pair(std::string(instance.unique_id), &instance));
+    }
+  }
+}
+
+// Writes summary of annotation instances to a file. Returns true if successful.
+bool WriteSummaryFile(const std::vector<Instance>& instances,
+                      const base::FilePath& file_path) {
+  FILE* output_file = fopen(file_path.MaybeAsASCII().c_str(), "wt");

[msramek, 2017/02/21 15:57:54]

Ditto; should we use base::File?

[Ramin Halavati, 2017/02/23 12:09:49]

Done.

+  if (!output_file) {
+    printf(" Could not create output file: %s",
+           file_path.MaybeAsASCII().c_str());
+    return false;
+  }
+  for (const auto& instance : instances) {
+    fprintf(output_file, "Unique ID: %s\n", instance.unique_id.c_str());
+    if (instance.state == Instance::kUndefined)
+      fprintf(output_file, "WARNING: Undefined annotation.\n");
+    else if (instance.state == Instance::kDuplicate)
+      fprintf(output_file, "WARNING: Duplicate unique id.\n");
+    fprintf(output_file, "Calling Function: %s\n", instance.callee.c_str());
+    std::string temp;
+    google::protobuf::TextFormat::PrintToString(instance.proto, &temp);
+    fprintf(output_file, "Content:\n%s\n", temp.c_str());
+    fprintf(output_file, "------------------------------------------------\n");

[msramek, 2017/02/21 15:57:54]

How wide can the output above be? I think this separator should be longer, say,
80 chars.

[Ramin Halavati, 2017/02/23 12:09:50]

Done.

+  }
+  fclose(output_file);
+  printf("Output file %s written for %i instances.\n",
+         file_path.MaybeAsASCII().c_str(), (int)instances.size());
+  return true;
+}
+
+#ifdef _WIN32
+int wmain(int argc, wchar_t* argv[]) {
+#else
+int main(int argc, char* argv[]) {
+#endif
+  // Parse switches.
+  base::CommandLine command_line = base::CommandLine(argc, argv);
+  if (command_line.HasSwitch("help") || command_line.HasSwitch("h") ||
+      argc == 1) {
+    printf(
+        "Usage: traffic_annotation_auditor [OPTION]... [path_filter]...\n"
+        "Extracts network traffic annotations from source files. If path "
+        "filter(s) are specified, only those directories of the source "
+        "will be analyzed.\n"
+        "Options:\n"
+        "  -h, --help              Shows help.\n"
+        "  --build_dir             Path to the build directory from which the\n"
+        "                            annotations will be extracted.\n"
+        "  --extractor_output_dir  Path to the directory that extracted\n"
+        "                            partial files will be written to. Will\n"
+        "                            be automatically generated and deleted\n"
+        "                            if not specified.\n"
+        "  --extracted_input_dir   Path to a directory where extracted\n"
+        "                            partial annotations are already stored.\n"
+        "                            If specified, build directory will be\n"
+        "                            ignored.\n"
+        "  --summary_file          Path to an output file with summary of\n"
+        "                            extracted annotations.\n"
+        "Example:\n"
+        "  traffic_annotation_auditor --build_dir=out/Debug\n"
+        "    --summary_file=report.txt\n");
+    return 1;
+  }
+  base::FilePath extracted_files_dir =
+      command_line.GetSwitchValuePath("extracted_input_dir");
+  base::FilePath build_dir = command_line.GetSwitchValuePath("build_dir");
+  base::FilePath extractor_output_dir =
+      command_line.GetSwitchValuePath("extractor_output_dir");
+  base::FilePath summary_file = command_line.GetSwitchValuePath("summary_file");
+  base::ScopedTempDir temp_dir;
+
+  if (summary_file.empty())
+    fprintf(stderr, "WARNING: Output file not specified.\n");
+
+  // Extract annotations.
+  if (extracted_files_dir.empty()) {
+    // Get build directory, if it is empty issue an error.
+    if (build_dir.empty()) {
+      fprintf(
+          stderr,
+          "You must either specify build directory to run clang "
+          "tool and extract annotations, or specify extracted files's input "
+          "directory where already extracted files exist.\n");
+      return 1;
+    }
+    // If output directory is not provided, create a temporary one.
+    if (extractor_output_dir.empty()) {
+      if (!temp_dir.CreateUniqueTempDirUnderPath(build_dir)) {
+        fprintf(stderr, "Could not create temporary directory in %s.\n",
+                build_dir.MaybeAsASCII().c_str());
+        return 1;
+      }
+      extractor_output_dir = temp_dir.GetPath();
+    } else {
+      // Ensure given directory is empty.
+      if (!base::FileEnumerator(extractor_output_dir, false,
+                                base::FileEnumerator::FILES)
+               .Next()
+               .empty()) {
+        fprintf(stderr, "Output directory (%s) should be empty.\n",
+                extractor_output_dir.MaybeAsASCII().c_str());
+        return 1;
+      }
+    }
+
+    // Get path filters, if none is provided, just add all.
+    base::CommandLine::StringVector path_filters = command_line.GetArgs();
+    if (!path_filters.size()) {
+      base::FilePath temp;
+      path_filters.push_back(temp.AppendASCII("./").value().c_str());
+    }
+
+    if (!RunClangTool(command_line.GetProgram().DirName().AppendASCII("../.."),

[msramek, 2017/02/21 15:57:55]

Please add a comment to explain why two directories up.

[Ramin Halavati, 2017/02/23 12:09:49]

Done.

+                      build_dir, extractor_output_dir, path_filters)) {
+      return 1;
+    }
+
+    extracted_files_dir = extractor_output_dir;
+  }
+
+  // Read all extracted files.
+  std::vector<Instance> instances;
+  ReadExtractedFiles(extracted_files_dir, &instances);
+
+  if (instances.empty()) {
+    fprintf(stderr, "Could not read any file.\n");
+    return 1;
+  } else {
+    MarkRepeatedUniqueIds(&instances);
+  }
+
+  // Create Summary file if requested.
+  if (!summary_file.empty() && !WriteSummaryFile(instances, summary_file))
+    return 1;
+
+  return 0;
+}