dump_modemlog: include log mask history files in dumpstate am: 52d632cbe7

Original change: https://googleplex-android-review.googlesource.com/c/device/google/gs-common/+/23850677

Change-Id: I0afa783bf297bdb3e991be8b246abcf63550db3e
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

modem/Android.bp

@@ -9,6 +9,13 @@ sh_binary {
     sub_dir: "dump",
 }
 
+cc_defaults {
+    name: "dump_modemlog_defaults",
+    srcs: ["modem_log_dumper.cpp"],
+    local_include_dirs: ["include"],
+    shared_libs: ["liblog"],
+}
+
 cc_binary {
     name: "dump_modemlog",
     srcs: ["dump_modemlog.cpp"],
@@ -22,7 +29,16 @@ cc_binary {
         "libdump",
         "liblog",
     ],
+    defaults: ["dump_modemlog_defaults"],
     vendor: true,
     relative_install_path: "dump",
 }
 
+cc_test {
+    name: "dump_modemlog_test",
+    srcs: ["test/*.cpp"],
+    defaults: ["dump_modemlog_defaults"],
+    local_include_dirs: ["test/include"],
+    static_libs: ["libgmock"],
+    vendor: true,
+}

modem/dump_modemlog.cpp

@@ -13,52 +13,61 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-#include <dump/pixel_dump.h>
 #include <android-base/properties.h>
-#include <log/log.h>
+#include <dump/pixel_dump.h>
 
-#define MODEM_LOGGING_PERSIST_PROPERTY "persist.vendor.sys.modem.logging.enable"
+#include "dumper.h"
-#define MODEM_LOGGING_PROPERTY "vendor.sys.modem.logging.enable"
+#include "modem_log_dumper.h"
-#define MODEM_LOGGING_STATUS_PROPERTY "vendor.sys.modem.logging.status"
+
-#define MODEM_LOGGING_NUMBER_BUGREPORT_PROPERTY "persist.vendor.sys.modem.logging.br_num"
+namespace modem {
-#define MODEM_LOGGING_PATH_PROPERTY "vendor.sys.modem.logging.log_path"
+namespace logging {
-#define MODEM_SIM_DIRECTORY "/data/vendor/radio/sim/"
+
-#define MODEM_LOG_PREFIX "sbuff_"
+/**
-#define SIM_POWERON_LOG_PREFIX "sim_poweron_log_"
+ * @brief Implementation of AndroidPropertyManager that directly forwards to
+ * android base methods.
+ */
+class AndroidPropertyManagerImpl : public AndroidPropertyManager {
+ public:
+  bool GetBoolProperty(const std::string& key, bool default_value) override {
+    return android::base::GetBoolProperty(key, default_value);
+  };
+
+  std::string GetProperty(const std::string& key,
+                          const std::string& default_value) override {
+    return android::base::GetProperty(key, default_value);
+  };
+  int GetIntProperty(const std::string& key, int default_value) override {
+    return android::base::GetIntProperty(key, default_value);
+  };
+  void SetProperty(const std::string& key, const std::string& value) override {
+    android::base::SetProperty(key, value);
+  };
+};
+
+/**
+ * @brief Implementation of Dumper that directly forwards to their corresponding
+ * dumpstate methods.
+ */
+class DumperImpl : public Dumper {
+ public:
+  void DumpLogs(const LogDumpInfo& log_dump_info) override {
+    dumpLogs(log_dump_info.src_dir.data(), log_dump_info.dest_dir.data(),
+             log_dump_info.limit, log_dump_info.prefix.data());
+  }
+  void CopyFile(const FileCopyInfo& file_copy_info) override {
+    copyFile(file_copy_info.src_dir.data(), file_copy_info.dest_dir.data());
+  }
+};
+
+}  // namespace logging
+}  // namespace modem
 
 int main() {
-    bool modemLogEnabled = ::android::base::GetBoolProperty(MODEM_LOGGING_PERSIST_PROPERTY, false);
+  modem::logging::DumperImpl dumper_impl;
-    if (modemLogEnabled && ::android::base::GetProperty(MODEM_LOGGING_PATH_PROPERTY, "") == MODEM_LOG_DIRECTORY) {
+  modem::logging::AndroidPropertyManagerImpl android_property_manager_impl;
-        bool modemLogStarted = ::android::base::GetBoolProperty(MODEM_LOGGING_STATUS_PROPERTY, false);
+  modem::logging::ModemLogDumper modem_log_dumper(
-        int maxFileNum = ::android::base::GetIntProperty(MODEM_LOGGING_NUMBER_BUGREPORT_PROPERTY, 100);
+      dumper_impl, android_property_manager_impl);
 
-        if (modemLogStarted) {
+  modem_log_dumper.DumpModemLogs();
-            ::android::base::SetProperty(MODEM_LOGGING_PROPERTY, "false");
-            ALOGD("Stopping modem logging...\n");
-        } else {
-            ALOGD("modem logging is not running\n");
-        }
-
-        for (int i = 0; i < 15; i++) {
-            if (!::android::base::GetBoolProperty(MODEM_LOGGING_STATUS_PROPERTY, false)) {
-                ALOGD("modem logging stopped\n");
-                sleep(1);
-                break;
-            }
-            sleep(1);
-        }
-
-        dumpLogs(MODEM_LOG_DIRECTORY, BUGREPORT_PACKING_DIR, maxFileNum, MODEM_LOG_PREFIX);
-
-        if (modemLogStarted) {
-            ALOGD("Restarting modem logging...\n");
-            ::android::base::SetProperty(MODEM_LOGGING_PROPERTY, "true");
-        }
-    }
-
-    dumpLogs("/data/vendor/radio/extended_logs", BUGREPORT_PACKING_DIR, 20, "extended_log_");
-    dumpLogs(MODEM_SIM_DIRECTORY, BUGREPORT_PACKING_DIR, 1, SIM_POWERON_LOG_PREFIX);
-    copyFile("/mnt/vendor/efs/nv_normal.bin", "/data/vendor/radio/logs/always-on/all_logs/nv_normal.bin");
-    copyFile("/mnt/vendor/efs/nv_protected.bin", "/data/vendor/radio/logs/always-on/all_logs/nv_protected.bin");
   return 0;
 }

modem/include/android_property_manager.h

@@ -0,0 +1,21 @@
+#pragma once
+
+#include <string>
+
+namespace modem {
+namespace logging {
+
+/**
+ * @brief Interface for interacting with Android System Properties.
+ */
+class AndroidPropertyManager {
+ public:
+  virtual ~AndroidPropertyManager() = default;
+  virtual bool GetBoolProperty(const std::string& key, bool default_value);
+  virtual std::string GetProperty(const std::string& key,
+                                  const std::string& default_value);
+  virtual int GetIntProperty(const std::string& key, int default_value);
+  virtual void SetProperty(const std::string& key, const std::string& value);
+};
+}  // namespace logging
+}  // namespace modem

modem/include/dumper.h

@@ -0,0 +1,71 @@
+#pragma once
+
+#include <ostream>
+#include <string_view>
+
+namespace modem {
+namespace logging {
+
+/**
+ * @brief Data object for information about dumpings logs.
+ *
+ * @param src_dir is a const char* containing the path to the directory to be
+ copied.
+ * @param dest_dir is a const char* containing the path to the directory that
+ the contents of the source directory should be copied to.
+ * @param limit is an int of the maximum number of files to copy.
+ * @param prefix is a const char* containing a prefix that all files to be
+ copied should have.
+*/
+struct LogDumpInfo {
+  const std::string_view src_dir;
+  const std::string_view dest_dir;
+  int limit;
+  const std::string_view prefix;
+
+  friend bool operator==(const LogDumpInfo& lhs, const LogDumpInfo& rhs) {
+    return (lhs.limit == rhs.limit) && (lhs.src_dir == rhs.src_dir) &&
+           (lhs.dest_dir == rhs.dest_dir) && (lhs.prefix == rhs.prefix);
+  }
+
+  // Do I have to use .data() here?
+  friend std::ostream& operator<<(std::ostream& os, const LogDumpInfo& obj) {
+    os << "src_dir: " << obj.src_dir << ", dest_dir: " << obj.dest_dir
+       << ", limit: " << obj.limit << ", prefix: " << obj.prefix;
+    return os;
+  }
+};
+
+/**
+ * @brief Data object for information about dumpings logs.
+ *
+ * @param src_dir is a const char* containing the path to a file to be copied.
+ * @param dest_dir is a const char* containing the destination path for the file
+ * to be copied to.
+ */
+struct FileCopyInfo {
+  const std::string_view src_dir;
+  const std::string_view dest_dir;
+
+  friend bool operator==(const FileCopyInfo& lhs, const FileCopyInfo& rhs) {
+    return (lhs.src_dir == rhs.src_dir) && (lhs.dest_dir == rhs.dest_dir);
+  }
+
+  // Do I have to add .data() here?
+  friend std::ostream& operator<<(std::ostream& os, const FileCopyInfo& obj) {
+    os << "src_dir: " << obj.src_dir << ", dest_dir: " << obj.dest_dir;
+    return os;
+  }
+};
+
+/**
+ * @brief Interface for dumping modem logs and files.
+ */
+class Dumper {
+ public:
+  virtual ~Dumper() = default;
+  virtual void DumpLogs(const LogDumpInfo& log_dump_info);
+  virtual void CopyFile(const FileCopyInfo& file_copy_info);
+};
+}  // namespace logging
+}  // namespace modem

modem/include/modem_log_constants.h

@@ -0,0 +1,56 @@
+#pragma once
+#include <string>
+
+#include "dumper.h"
+
+namespace modem {
+namespace logging {
+
+// Modem related Android System Properties
+
+// Controls triggering `modem_logging_start` and `modem_logging_stop`.
+inline constexpr static std::string_view kModemLoggingEnabledProperty =
+    "vendor.sys.modem.logging.enable";
+// Signals the current modem logging state. This will be set to
+// `vendor.sys.modem.logging.enable` when `modem_log_start` or `modem_log_stop`
+// terminates.
+inline constexpr static std::string_view kModemLoggingStatusProperty =
+    "vendor.sys.modem.logging.status";
+// Int which specifies how many files to include in the bugreport.
+inline constexpr static std::string_view kModemLoggingNumberBugreportProperty =
+    "persist.vendor.sys.modem.logging.br_num";
+// Signals the current location that is being logged to. This can be used to
+// determine the logging type.
+inline constexpr static std::string_view kModemLoggingPathProperty =
+    "vendor.sys.modem.logging.log_path";
+
+// Bugreport constants
+inline constexpr static int kDefaultBugreportNumberFiles = 100;
+inline constexpr static std::string_view kModemAlwaysOnLogDirectory =
+    "/data/vendor/radio/logs/always-on";
+inline constexpr static std::string_view kModemLogPrefix = "sbuff_";
+inline constexpr static std::string_view kBugreportPackingDirectory =
+    "/data/vendor/radio/logs/always-on/all_logs";
+
+inline constexpr static LogDumpInfo kLogDumpInfo[] = {
+    {.src_dir = "/data/vendor/radio/extended_logs",
+     .dest_dir = kBugreportPackingDirectory,
+     .limit = 20,
+     .prefix = "extended_log_"},
+    {.src_dir = "/data/vendor/radio/sim/",
+     .dest_dir = kBugreportPackingDirectory,
+     .limit = 1,
+     .prefix = "sim_poweron_log_"},
+    {.src_dir = "data/vendor/radio/logs/history",
+     .dest_dir = kBugreportPackingDirectory,
+     .limit = 2,
+     .prefix = "Logging"}};
+
+constexpr static FileCopyInfo kFileCopyInfo[] = {
+    {.src_dir = "/mnt/vendor/efs/nv_normal.bin",
+     .dest_dir = "/data/vendor/radio/logs/always-on/all_logs/nv_normal.bin"},
+    {.src_dir = "/mnt/vendor/efs/nv_protected.bin",
+     .dest_dir =
+         "/data/vendor/radio/logs/always-on/all_logs/nv_protected.bin"}};
+}  // namespace logging
+}  // namespace modem

modem/include/modem_log_dumper.h

@@ -0,0 +1,81 @@
+#pragma once
+
+#include "android_property_manager.h"
+#include "dumper.h"
+
+namespace modem {
+namespace logging {
+
+/**
+ * @brief Responsible for dumping all relevant modem logs.
+ */
+class ModemLogDumper {
+ public:
+  ModemLogDumper(Dumper& dumper,
+                 AndroidPropertyManager& android_property_manager)
+      : dumper_(dumper), android_property_manager_(android_property_manager){};
+
+  /**
+   * @brief Dumps modem related logs and persistent files to bugreport.
+   *
+   * If PILOT and On Demand Logging are both not enabled, this method will
+   * attempt to stop modem logging, copy over the logs, and then restart so that
+   * the original logging enabled / disabled state is preserved. Additionally,
+   * all directories specified in `kLogDumpInfo` and all files in
+   * `kFileCopyInfo` will be included.
+   */
+  void DumpModemLogs();
+
+ private:
+  /**
+   * @brief Checks modem logging status property to assert if logging is
+   * running or not.
+   */
+  bool isModemLoggingRunning();
+
+  /**
+   * @brief Checks if On Demand Logging or PILOT Logging is enabled.
+   *
+   * If either of them are enabled, then the `log_path` property will no longer
+   * point to the always on logging directory.
+   */
+  bool allowedToStopModemLogging();
+
+  /**
+   * @brief Stops modem logging.
+   *
+   * This sets the modem logging property which in turn triggers
+   * modem_logging_control's modem_logging_stop service. Modem logging isn't
+   * guaranteed to have stopped after this call, so it's necessary to poll the
+   * status property to ensure it's stopped before proceeding.
+   */
+  void stopModemLogging();
+
+  /**
+   * @brief Polls modem logging status property to ensure modem logging has
+   * stopped.
+   *
+   * Even after the property is confirmed to be false, it will continue to
+   * sleep for a second to ensure that the modem_logging_stop service has exited
+   * properly.
+   */
+  void waitForStopModemLogging();
+
+  /**
+   * @brief Starts modem logging.
+   *
+   * This sets the modem logging property which in turn triggers
+   * modem_logging_control's modem_logging_start service. Modem logging isn't
+   * guaranteed to have started after this call, so it's necessary to poll the
+   * status property to ensure it's started before proceeding to guarantee
+   * success.
+   */
+  void startModemLogging();
+
+ private:
+  Dumper& dumper_;
+  AndroidPropertyManager& android_property_manager_;
+};
+
+}  // namespace logging
+}  // namespace modem

modem/modem_log_dumper.cpp

@@ -0,0 +1,80 @@
+#include "modem_log_dumper.h"
+
+#include <log/log.h>
+
+#include "dumper.h"
+#include "modem_log_constants.h"
+
+namespace modem {
+namespace logging {
+
+void ModemLogDumper::DumpModemLogs() {
+  bool shouldRestartModemLogging =
+      allowedToStopModemLogging() && isModemLoggingRunning();
+  int maxFileNum = android_property_manager_.GetIntProperty(
+      kModemLoggingNumberBugreportProperty.data(),
+      kDefaultBugreportNumberFiles);
+
+  if (shouldRestartModemLogging) {
+    // If modem logging is running at time of bugreport, it needs to be stopped
+    // to ensure that the most recent logs are included in the bugreport. If
+    // this command fails, only older log files will be included, as seen in
+    // b/289435256.
+    stopModemLogging();
+    waitForStopModemLogging();
+  } else {
+    ALOGD("modem logging is not running\n");
+  }
+
+  dumper_.DumpLogs({kModemAlwaysOnLogDirectory, kBugreportPackingDirectory,
+                    maxFileNum, kModemLogPrefix});
+
+  if (shouldRestartModemLogging) {
+    startModemLogging();
+  }
+
+  for (const LogDumpInfo& log_dump_info : kLogDumpInfo) {
+    dumper_.DumpLogs(log_dump_info);
+  }
+
+  for (const FileCopyInfo& file_copy_info : kFileCopyInfo) {
+    dumper_.CopyFile(file_copy_info);
+  }
+};
+
+bool ModemLogDumper::isModemLoggingRunning() {
+  return android_property_manager_.GetBoolProperty(
+      kModemLoggingStatusProperty.data(), false);
+}
+
+bool ModemLogDumper::allowedToStopModemLogging() {
+  return android_property_manager_.GetProperty(kModemLoggingPathProperty.data(),
+                                               /*default_value=*/"") ==
+         kModemAlwaysOnLogDirectory;
+}
+
+void ModemLogDumper::stopModemLogging() {
+  android_property_manager_.SetProperty(kModemLoggingEnabledProperty.data(),
+                                        "false");
+  ALOGD("Stopping modem logging...\n");
+}
+
+void ModemLogDumper::waitForStopModemLogging() {
+  // TODO(b/289582966) improve stop logging mechanism to not use sleep
+  for (int i = 0; i < 15; i++) {
+    if (!isModemLoggingRunning()) {
+      ALOGD("modem logging stopped\n");
+      sleep(1);
+      break;
+    }
+    sleep(1);
+  }
+}
+
+void ModemLogDumper::startModemLogging() {
+  ALOGD("Restarting modem logging...\n");
+  android_property_manager_.SetProperty(kModemLoggingEnabledProperty.data(),
+                                        "true");
+}
+}  // namespace logging
+}  // namespace modem

modem/test/include/fake_android_property_manager.h

@@ -0,0 +1,77 @@
+
+
+#include <map>
+#include <string>
+#include <string_view>
+
+#include "android_property_manager.h"
+#include "modem_log_constants.h"
+
+namespace modem {
+namespace logging {
+
+/**
+ * @brief Fake Implementation of AndroidPropertyManager that mocks some of the
+ * property changing behaviour from pixellogger's `modem_logging_control`.
+ */
+class FakeAndroidPropertyManager : public AndroidPropertyManager {
+ public:
+  inline constexpr static std::string_view kTruthString = "true";
+  inline constexpr static std::string_view kFalseString = "false";
+
+  bool GetBoolProperty(const std::string& key, bool default_value) override {
+    return MapContainsKey(key)
+               ? GetPropertyInternal(key) == kTruthString
+               : default_value;
+  };
+
+  std::string GetProperty(const std::string& key,
+                          const std::string& default_value) override {
+    return MapContainsKey(key) ? GetPropertyInternal(key) : default_value;
+    ;
+  };
+
+  int GetIntProperty(const std::string& key, int default_value) override {
+    return MapContainsKey(key) ? std::stoi(GetPropertyInternal(key))
+                               : default_value;
+  };
+
+  /**
+   * This function needs to copy the behaviour of `modem_logging_control` to
+   * ensure that the right properties are being set in order.
+   *
+   * More specifically, this function will also set the
+   * `kModemLoggingStatusProperty` whenever `kModemLoggingEnabledProperty` is
+   * set to simulate modem logging stopping / starting.
+   */
+  void SetProperty(const std::string& key, const std::string& value) override {
+    if (key == kModemLoggingEnabledProperty) {
+      property_map_[kModemLoggingStatusProperty.data()] = value;
+
+      // need to track if modem logging has restarted or not
+      if (value == kFalseString) {
+        modem_logging_has_been_off_ = true;
+      }
+      if (modem_logging_has_been_off_ && (value == kTruthString)) {
+        modem_logging_has_restarted_ = true;
+      }
+    }
+    property_map_[key] = value;
+  };
+
+  bool ModemLoggingHasRestarted() { return modem_logging_has_restarted_; }
+
+ private:
+  bool MapContainsKey(const std::string& key) {
+    return property_map_.find(key) != property_map_.end();
+  }
+  std::string GetPropertyInternal(const std::string& key) {
+    return property_map_.find(key)->second;
+  }
+
+  std::map<std::string, std::string> property_map_;
+  bool modem_logging_has_been_off_ = false;
+  bool modem_logging_has_restarted_ = false;
+};
+}  // namespace logging
+}  // namespace modem

modem/test/modem_log_dumper_test.cpp

@@ -0,0 +1,106 @@
+#include "modem_log_dumper.h"
+
+#include <string_view>
+
+#include "dumper.h"
+#include "fake_android_property_manager.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace modem {
+namespace logging {
+namespace {
+
+using ::testing::Eq;
+
+inline constexpr static std::string_view kFakePilotLoggingPath =
+    "//pilot/logging/path";
+inline constexpr static std::string_view kFakeOnDemandLoggingPath =
+    "//on/demand/logging/path";
+inline constexpr static LogDumpInfo kAlwaysOnLogDumpInfo = {
+    kModemAlwaysOnLogDirectory, kBugreportPackingDirectory,
+    kDefaultBugreportNumberFiles, kModemLogPrefix};
+
+void StartModemLogging(
+    FakeAndroidPropertyManager& fake_android_property_manager) {
+  fake_android_property_manager.SetProperty(
+      kModemLoggingEnabledProperty.data(),
+      FakeAndroidPropertyManager::kTruthString.data());
+}
+
+class MockDumper : public Dumper {
+ public:
+  ~MockDumper() = default;
+  MOCK_METHOD(void, DumpLogs, (const LogDumpInfo&), (override));
+  MOCK_METHOD(void, CopyFile, (const FileCopyInfo&), (override));
+};
+
+class ModemLogDumperTest : public ::testing::Test {
+ protected:
+  ModemLogDumperTest()
+      : modem_log_dumper(mock_dumper, fake_android_property_manager) {}
+
+  void SetUp() override {
+    // set default logging mode to always on logging
+    fake_android_property_manager.SetProperty(
+        kModemLoggingPathProperty.data(), kModemAlwaysOnLogDirectory.data());
+  }
+
+  MockDumper mock_dumper;
+  FakeAndroidPropertyManager fake_android_property_manager;
+  ModemLogDumper modem_log_dumper;
+};
+
+TEST_F(ModemLogDumperTest, DumpLogsDumpsAllDirectoriesAndCopiesAllFiles) {
+  EXPECT_CALL(mock_dumper, DumpLogs(Eq(kAlwaysOnLogDumpInfo)));
+
+  for (const LogDumpInfo& log_dump_info : kLogDumpInfo) {
+    EXPECT_CALL(mock_dumper, DumpLogs(Eq(log_dump_info)));
+  }
+
+  for (const FileCopyInfo& fileCopyInfo : kFileCopyInfo) {
+    EXPECT_CALL(mock_dumper, CopyFile(Eq(fileCopyInfo)));
+  }
+
+  modem_log_dumper.DumpModemLogs();
+}
+
+TEST_F(ModemLogDumperTest, DumpLogsRestartModemLoggingWhenEnabled) {
+  StartModemLogging(fake_android_property_manager);
+
+  modem_log_dumper.DumpModemLogs();
+
+  EXPECT_TRUE(fake_android_property_manager.ModemLoggingHasRestarted());
+}
+
+TEST_F(ModemLogDumperTest, DumpLogsDoesNotRestartModemLoggingWhenDisabled) {
+  modem_log_dumper.DumpModemLogs();
+
+  EXPECT_FALSE(fake_android_property_manager.ModemLoggingHasRestarted());
+}
+
+TEST_F(ModemLogDumperTest, DumpLogsDoesNotRestartModemLoggingWhenPilotEnabled) {
+  // Enable PILOT
+  fake_android_property_manager.SetProperty(kModemLoggingPathProperty.data(),
+                                            kFakePilotLoggingPath.data());
+  StartModemLogging(fake_android_property_manager);
+
+  modem_log_dumper.DumpModemLogs();
+
+  EXPECT_FALSE(fake_android_property_manager.ModemLoggingHasRestarted());
+}
+
+TEST_F(ModemLogDumperTest,
+       DumpLogsDoesNotRestartModemLoggingWhenOnDemandLoggingEnabled) {
+  // Enable On Demand Logging
+  fake_android_property_manager.SetProperty(kModemLoggingPathProperty.data(),
+                                            kFakeOnDemandLoggingPath.data());
+  StartModemLogging(fake_android_property_manager);
+
+  modem_log_dumper.DumpModemLogs();
+
+  EXPECT_FALSE(fake_android_property_manager.ModemLoggingHasRestarted());
+}
+}  // namespace
+}  // namespace logging
+}  // namespace modem