Refactoring the SysInfoProvider.

chrome/browser/chromeos/system_logs/lsbrelease_log_source.cc

+// Copyright (c) 2012 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.
+
+#include "chrome/browser/chromeos/system_logs/lsbrelease_log_source.h"
+
+#include <string>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "base/string_util.h"
+#include "content/public/browser/browser_thread.h"
+
+using content::BrowserThread;
+
+namespace {
+
+// TODO(tudalex): Replace this code with a more simpler code based on a string
+// tokenizer, this code was copied over from the SysLogsProvide class.

[satorux1, 2012/08/14 05:51:52]

Ugh, copy-and-pasting this large amount of code is bad. Please fix this now
rather than later.

[tudalex(Chromium), 2012/08/15 01:54:33]

Done.

+const char kMultilineQuote[] = "\"\"\"";
+const char kNewLineChars[] = "\r\n";
+const char kInvalidLogEntry[] = "<invalid characters in log entry>";
+const char kEmptyLogEntry[] = "<no value>";
+
+// Reads a key from the input string erasing the read values + delimiters read
+// from the initial string
+std::string ReadKey(std::string* data) {
+  size_t equal_sign = data->find("=");
+  if (equal_sign == std::string::npos)
+    return std::string("");
+  std::string key = data->substr(0, equal_sign);
+  data->erase(0, equal_sign);
+  if (data->size() > 0) {
+    // erase the equal to sign also
+    data->erase(0, 1);
+    return key;
+  }
+  return std::string();
+}
+
+// Reads a value from the input string; erasing the read values from
+// the initial string; detects if the value is multiline and reads
+// accordingly
+std::string ReadValue(std::string* data) {
+  // Trim the leading spaces and tabs. In order to use a multi-line
+  // value, you have to place the multi-line quote on the same line as
+  // the equal sign.
+  //
+  // Why not use TrimWhitespace? Consider the following input:
+  //
+  // KEY1=
+  // KEY2=VALUE
+  //
+  // If we use TrimWhitespace, we will incorrectly trim the new line
+  // and assume that KEY1's value is "KEY2=VALUE" rather than empty.
+  TrimString(*data, " \t", data);
+
+  // If multiline value
+  if (StartsWithASCII(*data, std::string(kMultilineQuote), false)) {
+    data->erase(0, strlen(kMultilineQuote));
+    size_t next_multi = data->find(kMultilineQuote);
+    if (next_multi == std::string::npos) {
+      // Error condition, clear data to stop further processing
+      data->erase();
+      return std::string();
+    }
+    std::string value = data->substr(0, next_multi);
+    data->erase(0, next_multi + 3);
+    return value;
+  } else {  // single line value
+    size_t endl_pos = data->find_first_of(kNewLineChars);
+    // if we don't find a new line, we just return the rest of the data
+    std::string value = data->substr(0, endl_pos);
+    data->erase(0, endl_pos);
+    return value;
+  }
+}
+
+// Gets the info from the /etc/lsb-release and parses it
+void ParseLSBRelease(chromeos::SystemLogsResponse* response) {

[satorux1, 2012/08/14 05:51:52]

Add DCHECK(response); followed by a blank line.

[tudalex(Chromium), 2012/08/15 01:54:33]

Done.

+  FilePath lsb_release_file("/etc/lsb-release");

[satorux1, 2012/08/14 05:51:52]

const

[tudalex(Chromium), 2012/08/15 01:54:33]

Done.

+  std::string data;
+  bool read_success = file_util::ReadFileToString(lsb_release_file,
+                                                      &data);
+  // if we were using an internal temp file, the user does not need the
+  // logs to stay past the ReadFile call - delete the file
+  if (!read_success) {
+    LOG(ERROR) << "Can't access /etc/lsb-release file.";
+    return;
+  }
+  while (data.length() > 0) {

[satorux1, 2012/08/14 05:51:52]

I think what you want to do is something like:

in_multi_line_value = false;
SplitString(data, '\n', &lines);
for each line:
  if in_multi_line_value:
    if line ends with """:
       remove """ from line;
       in_multi_line_value = false;
    value += line;
  else:
    SplitString(line, '=', &pair);
    key = pair[0];
    value = pair[1];
    if value starts with """:
       in_multi_line_value = true;
   if !in_multi_line_value:
    response[key] = value;

[tudalex(Chromium), 2012/08/15 01:54:33]

Talked to rkc@ and lsb-release actually doesn't have multi line values. The code
that was copied was used to parse all the older log sources. So I used
base::SplitStringIntoKeyValuePairs.

[satorux1, 2012/08/15 22:01:09]

so while you are working on the similar code here, could you fix the bad code in
the other file? Could you file a bug and fix in a separate patch? you don't have
to fix this in this patch.

[tudalex(Chromium), 2012/08/16 01:08:57]

The plan is that the old file is deprecated and the next CL's will remove that
file. It is still being used for Feedback and networking and also the code there
was handling multi line values.

+    std::string key = ReadKey(&data);
+    TrimWhitespaceASCII(key, TRIM_ALL, &key);
+    if (!key.empty()) {
+      std::string value = ReadValue(&data);
+      if (IsStringUTF8(value)) {
+        TrimWhitespaceASCII(value, TRIM_ALL, &value);
+        if (value.empty())
+          (*response)[key] = kEmptyLogEntry;
+        else
+          (*response)[key] = value;
+      } else {
+        LOG(WARNING) << "Invalid characters in system log entry: " << key;
+        (*response)[key] = kInvalidLogEntry;
+      }
+    } else {
+      // no more keys, we're done
+      break;
+    }
+  }
+}

[satorux1, 2012/08/14 05:51:52]

This function is complicated and can easily be unit tested. Please make it a
public static function, and write a unit test (yes, I know I asked you to put
this in anonymous namespace, but I take it back)

[tudalex(Chromium), 2012/08/15 01:54:33]

Done, but I had to split it into to so that I can unit test it.

+
+}  // namespace
+
+namespace chromeos {
+
+void LsbReleaseLogSource::Fetch(const SysLogsFetcherCallback& request) {
+  SystemLogsResponse* response = new SystemLogsResponse;
+  BrowserThread::PostBlockingPoolTaskAndReply(
+      FROM_HERE,
+      base::Bind(&ParseLSBRelease, response),
+      base::Bind(request,
+                 base::Owned(response)));
+}
+
+}  // namespace chromeos
+