Refactor: Move ReadEndOfFile function into feedback_util.

The function will be used by Lacros system log source.

Bug:1109387
TEST: Send feedback reports and verify nothing is changed.

Change-Id: Id1bf276a10d1389b3ea1872d03a561c97490eea7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2368153Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Miriam Zimmerman <mutexlox@chromium.org>
Commit-Queue: Jenny Zhang <jennyz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800393}

chrome/browser/chromeos/BUILD.gn

@@ -3533,7 +3533,6 @@ source_set("unit_tests") {
     "system/device_disabling_manager_unittest.cc",
     "system/procfs_util_unittest.cc",
     "system/user_removal_manager_unittest.cc",
-    "system_logs/debug_daemon_log_source_unittest.cc",
     "system_logs/shill_log_source_unittest.cc",
     "system_logs/single_debug_daemon_log_source_unittest.cc",
     "system_logs/single_log_file_log_source_unittest.cc",

chrome/browser/chromeos/system_logs/debug_daemon_log_source.cc

@@ -24,6 +24,7 @@
 #include "chromeos/cryptohome/cryptohome_parameters.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/debug_daemon/debug_daemon_client.h"
+#include "components/feedback/feedback_util.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
 #include "content/public/browser/browser_thread.h"
@@ -68,64 +69,6 @@ const int64_t kMaxLogSize = 1024 * 1024;
 
 }  // namespace
 
-bool ReadEndOfFile(const base::FilePath& path,
-                   std::string* contents,
-                   size_t max_size) {
-  if (!contents) {
-    LOG(ERROR) << "contents buffer is null.";
-    return false;
-  }
-
-  if (path.ReferencesParent()) {
-    LOG(ERROR) << "ReadEndOfFile can't be called on file paths with parent "
-                  "references.";
-    return false;
-  }
-
-  base::ScopedFILE fp(base::OpenFile(path, "r"));
-  if (!fp) {
-    PLOG(ERROR) << "Failed to open file " << path.value();
-    return false;
-  }
-
-  std::unique_ptr<char[]> chunk(new char[max_size]);
-  std::unique_ptr<char[]> last_chunk(new char[max_size]);
-  chunk[0] = '\0';
-  last_chunk[0] = '\0';
-
-  size_t total_bytes_read = 0;
-  size_t bytes_read = 0;
-
-  // Since most logs are not seekable, read until the end keeping tracking of
-  // last two chunks.
-  while ((bytes_read = fread(chunk.get(), 1, max_size, fp.get())) == max_size) {
-    total_bytes_read += bytes_read;
-    last_chunk.swap(chunk);
-    chunk[0] = '\0';
-  }
-  total_bytes_read += bytes_read;
-
-  if (total_bytes_read < max_size) {
-    // File is smaller than max_size
-    contents->assign(chunk.get(), bytes_read);
-  } else if (bytes_read == 0) {
-    // File is exactly max_size or a multiple of max_size
-    contents->assign(last_chunk.get(), max_size);
-  } else {
-    // Number of bytes to keep from last_chunk
-    size_t bytes_from_last = max_size - bytes_read;
-
-    // Shift left last_chunk by size of chunk and fit it in the back of
-    // last_chunk.
-    memmove(last_chunk.get(), last_chunk.get() + bytes_read, bytes_from_last);
-    memcpy(last_chunk.get() + bytes_from_last, chunk.get(), bytes_read);
-
-    contents->assign(last_chunk.get(), max_size);
-  }
-
-  return true;
-}
-
 // Reads the contents of the user log files listed in |kUserLogs| and adds them
 // to the |response| parameter.
 void ReadUserLogFiles(const std::vector<base::FilePath>& profile_dirs,
@@ -134,9 +77,9 @@ void ReadUserLogFiles(const std::vector<base::FilePath>& profile_dirs,
     std::string profile_prefix = "Profile[" + base::NumberToString(i) + "] ";
     for (const auto& log : kUserLogs) {
       std::string value;
-      const bool read_success =
-          ReadEndOfFile(profile_dirs[i].Append(log.log_file_relative_path),
-                        &value, kMaxLogSize);
+      const bool read_success = feedback_util::ReadEndOfFile(
+          profile_dirs[i].Append(log.log_file_relative_path), kMaxLogSize,
+          &value);
 
       if (read_success && value.length() == kMaxLogSize) {
         value.replace(0, strlen(kLogTruncated), kLogTruncated);

components/feedback/BUILD.gn

@@ -30,6 +30,7 @@ static_library("feedback") {
 
   deps = [
     "//base",
+    "//build:lacros_buildflags",
     "//components/feedback/proto",
     "//components/keyed_service/content",
     "//components/keyed_service/core",
@@ -53,6 +54,10 @@ source_set("unit_tests") {
     "feedback_uploader_unittest.cc",
     "redaction_tool_unittest.cc",
   ]
+  if (!is_win) {
+    sources += [ "feedback_util_unittest.cc" ]
+  }
+
   deps = [
     ":feedback",
     "//base",

components/feedback/feedback_util.cc

@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "components/feedback/feedback_report.h"
 #include "third_party/zlib/google/zip.h"
@@ -73,4 +74,67 @@ std::string LogsToString(const FeedbackCommon::SystemLogsMap& sys_info) {
   return syslogs_string;
 }
 
+// Note: This function is excluded from win build because its unit tests do
+// not pass on OS_WIN.
+// This function is only called on ChromeOS and Lacros build.
+// See https://crbug.com/1119560.
+#if !defined(OS_WIN)
+bool ReadEndOfFile(const base::FilePath& path,
+                   size_t max_size,
+                   std::string* contents) {
+  if (!contents) {
+    LOG(ERROR) << "contents buffer is null.";
+    return false;
+  }
+
+  if (path.ReferencesParent()) {
+    LOG(ERROR) << "ReadEndOfFile can't be called on file paths with parent "
+                  "references.";
+    return false;
+  }
+
+  base::ScopedFILE fp(base::OpenFile(path, "r"));
+  if (!fp) {
+    PLOG(ERROR) << "Failed to open file " << path.value();
+    return false;
+  }
+
+  std::unique_ptr<char[]> chunk(new char[max_size]);
+  std::unique_ptr<char[]> last_chunk(new char[max_size]);
+  chunk[0] = '\0';
+  last_chunk[0] = '\0';
+
+  size_t total_bytes_read = 0;
+  size_t bytes_read = 0;
+
+  // Since most logs are not seekable, read until the end keeping tracking of
+  // last two chunks.
+  while ((bytes_read = fread(chunk.get(), 1, max_size, fp.get())) == max_size) {
+    total_bytes_read += bytes_read;
+    last_chunk.swap(chunk);
+    chunk[0] = '\0';
+  }
+  total_bytes_read += bytes_read;
+  if (total_bytes_read < max_size) {
+    // File is smaller than max_size
+    contents->assign(chunk.get(), bytes_read);
+  } else if (bytes_read == 0) {
+    // File is exactly max_size or a multiple of max_size
+    contents->assign(last_chunk.get(), max_size);
+  } else {
+    // Number of bytes to keep from last_chunk
+    size_t bytes_from_last = max_size - bytes_read;
+
+    // Shift left last_chunk by size of chunk and fit it in the back of
+    // last_chunk.
+    memmove(last_chunk.get(), last_chunk.get() + bytes_read, bytes_from_last);
+    memcpy(last_chunk.get() + bytes_from_last, chunk.get(), bytes_read);
+
+    contents->assign(last_chunk.get(), max_size);
+  }
+
+  return true;
+}
+#endif  // !OS_WIN
+
 }  // namespace feedback_util

components/feedback/feedback_util.h

@@ -20,6 +20,16 @@ bool ZipString(const base::FilePath& filename,
 // creating a system_logs.txt file attached to feedback reports.
 std::string LogsToString(const FeedbackCommon::SystemLogsMap& sys_info);
 
+#if !defined(OS_WIN)
+// Returns true if the data from the file specified by |path| is read into
+// |contents| successfully.
+// If the file size is greater than |max_size| in bytes, the data will be
+// truncated to |max_size| and put in |contents|.
+bool ReadEndOfFile(const base::FilePath& path,
+                   size_t max_size,
+                   std::string* contents);
+#endif
+
 }  // namespace feedback_util
 
 #endif  // COMPONENTS_FEEDBACK_FEEDBACK_UTIL_H_

chrome/browser/chromeos/system_logs/debug_daemon_log_source_unittest.cc → components/feedback/feedback_util_unittest.cc

-// Copyright (c) 2020 The Chromium Authors. All rights reserved.
+// Copyright 2020 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/debug_daemon_log_source.h"
+#include "components/feedback/feedback_util.h"
 
 #include <string>
 
@@ -12,27 +12,29 @@
 #include "base/rand_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-namespace system_logs {
+namespace feedback_util {
 
-class DebugDaemonLogSourceTest : public ::testing::Test {
+// Note: This file is excluded from win build.
+// See https://crbug.com/1119560.
+class FeedbackUtilTest : public ::testing::Test {
  public:
   void SetUp() override { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); }
 
   base::ScopedTempDir temp_dir_;
 };
 
-TEST_F(DebugDaemonLogSourceTest, ReadEndOfFileEmpty) {
+TEST_F(FeedbackUtilTest, ReadEndOfFileEmpty) {
   std::string read_data("should be erased");
 
   base::FilePath file_path = temp_dir_.GetPath().Append("test_empty.txt");
 
   WriteFile(file_path, "", 0);
 
-  EXPECT_TRUE(ReadEndOfFile(file_path, &read_data, 10));
+  EXPECT_TRUE(feedback_util::ReadEndOfFile(file_path, 10, &read_data));
   EXPECT_EQ(0u, read_data.length());
 }
 
-TEST_F(DebugDaemonLogSourceTest, ReadEndOfFileSmall) {
+TEST_F(FeedbackUtilTest, ReadEndOfFileSmall) {
   const char kTestData[] = "0123456789";  // Length of 10
   std::string read_data;
 
@@ -41,27 +43,27 @@ TEST_F(DebugDaemonLogSourceTest, ReadEndOfFileSmall) {
   WriteFile(file_path, kTestData, strlen(kTestData));
 
   read_data.clear();
-  EXPECT_TRUE(ReadEndOfFile(file_path, &read_data, 15));
+  EXPECT_TRUE(feedback_util::ReadEndOfFile(file_path, 15, &read_data));
   EXPECT_EQ(kTestData, read_data);
 
   read_data.clear();
-  EXPECT_TRUE(ReadEndOfFile(file_path, &read_data, 10));
+  EXPECT_TRUE(feedback_util::ReadEndOfFile(file_path, 10, &read_data));
   EXPECT_EQ(kTestData, read_data);
 
   read_data.clear();
-  EXPECT_TRUE(ReadEndOfFile(file_path, &read_data, 2));
+  EXPECT_TRUE(feedback_util::ReadEndOfFile(file_path, 2, &read_data));
   EXPECT_EQ("89", read_data);
 
   read_data.clear();
-  EXPECT_TRUE(ReadEndOfFile(file_path, &read_data, 3));
+  EXPECT_TRUE(feedback_util::ReadEndOfFile(file_path, 3, &read_data));
   EXPECT_EQ("789", read_data);
 
   read_data.clear();
-  EXPECT_TRUE(ReadEndOfFile(file_path, &read_data, 5));
+  EXPECT_TRUE(feedback_util::ReadEndOfFile(file_path, 5, &read_data));
   EXPECT_EQ("56789", read_data);
 }
 
-TEST_F(DebugDaemonLogSourceTest, ReadEndOfFileWithZeros) {
+TEST_F(FeedbackUtilTest, ReadEndOfFileWithZeros) {
   const size_t test_size = 10;
   std::string test_data("abcd\0\0\0\0hi", test_size);
   std::string read_data;
@@ -71,27 +73,27 @@ TEST_F(DebugDaemonLogSourceTest, ReadEndOfFileWithZeros) {
   WriteFile(file_path, test_data.data(), test_size);
 
   read_data.clear();
-  EXPECT_TRUE(ReadEndOfFile(file_path, &read_data, 15));
+  EXPECT_TRUE(feedback_util::ReadEndOfFile(file_path, 15, &read_data));
   EXPECT_EQ(test_data, read_data);
 
   read_data.clear();
-  EXPECT_TRUE(ReadEndOfFile(file_path, &read_data, 10));
+  EXPECT_TRUE(feedback_util::ReadEndOfFile(file_path, 10, &read_data));
   EXPECT_EQ(test_data, read_data);
 
   read_data.clear();
-  EXPECT_TRUE(ReadEndOfFile(file_path, &read_data, 2));
+  EXPECT_TRUE(feedback_util::ReadEndOfFile(file_path, 2, &read_data));
   EXPECT_EQ(test_data.substr(test_size - 2, 2), read_data);
 
   read_data.clear();
-  EXPECT_TRUE(ReadEndOfFile(file_path, &read_data, 3));
+  EXPECT_TRUE(feedback_util::ReadEndOfFile(file_path, 3, &read_data));
   EXPECT_EQ(test_data.substr(test_size - 3, 3), read_data);
 
   read_data.clear();
-  EXPECT_TRUE(ReadEndOfFile(file_path, &read_data, 5));
+  EXPECT_TRUE(feedback_util::ReadEndOfFile(file_path, 5, &read_data));
   EXPECT_EQ(test_data.substr(test_size - 5, 5), read_data);
 }
 
-TEST_F(DebugDaemonLogSourceTest, ReadEndOfFileMedium) {
+TEST_F(FeedbackUtilTest, ReadEndOfFileMedium) {
   std::string test_data = base::RandBytesAsString(10000);  // 10KB data
   std::string read_data;
 
@@ -102,24 +104,24 @@ TEST_F(DebugDaemonLogSourceTest, ReadEndOfFileMedium) {
   WriteFile(file_path, test_data.data(), test_size);
 
   read_data.clear();
-  EXPECT_TRUE(ReadEndOfFile(file_path, &read_data, 15000));
+  EXPECT_TRUE(feedback_util::ReadEndOfFile(file_path, 15000, &read_data));
   EXPECT_EQ(test_data, read_data);
 
   read_data.clear();
-  EXPECT_TRUE(ReadEndOfFile(file_path, &read_data, 10000));
+  EXPECT_TRUE(feedback_util::ReadEndOfFile(file_path, 10000, &read_data));
   EXPECT_EQ(test_data, read_data);
 
   read_data.clear();
-  EXPECT_TRUE(ReadEndOfFile(file_path, &read_data, 1000));
+  EXPECT_TRUE(feedback_util::ReadEndOfFile(file_path, 1000, &read_data));
   EXPECT_EQ(test_data.substr(test_size - 1000, 1000), read_data);
 
   read_data.clear();
-  EXPECT_TRUE(ReadEndOfFile(file_path, &read_data, 300));
+  EXPECT_TRUE(feedback_util::ReadEndOfFile(file_path, 300, &read_data));
   EXPECT_EQ(test_data.substr(test_size - 300, 300), read_data);
 
   read_data.clear();
-  EXPECT_TRUE(ReadEndOfFile(file_path, &read_data, 175));
+  EXPECT_TRUE(feedback_util::ReadEndOfFile(file_path, 175, &read_data));
   EXPECT_EQ(test_data.substr(test_size - 175, 175), read_data);
 }
 
-}  // namespace system_logs
+}  // namespace feedback_util