From 3ed60cec02e9917141f3fb854ebf8edf9f351e8d Mon Sep 17 00:00:00 2001
From: kierancyphus <kierancyphus@google.com>
Date: Fri, 10 Nov 2023 14:40:18 +0800
Subject: [PATCH 1/6] dump_modemlog: move all files to subdirectory

Since radioext has already moved to this folder, it doesn't make sense
for the base folder to be dump_modemlog. This change moves it to its
own subfolder so that we can also add more in the future.

Test: build and flash, trigger bugreport and check modem logs are there
Bug: 302435001
Change-Id: Ia83378074068526023f591d63b1e5ac4700b8103
---
 modem/{ => dump_modemlog}/Android.bp             | 16 ++++++++--------
 modem/{ => dump_modemlog}/dump_modem.sh          |  0
 modem/{ => dump_modemlog}/dump_modemlog.cpp      | 13 ++++++-------
 modem/dump_modemlog/dump_modemlog.mk             |  5 +++++
 .../include/android_property_manager.h           |  7 +++----
 modem/{ => dump_modemlog}/include/dumper.h       |  7 +++----
 .../include/modem_log_constants.h                |  7 +++----
 .../include/modem_log_dumper.h                   |  6 ++----
 modem/{ => dump_modemlog}/modem_log_dumper.cpp   |  7 +++----
 .../modem_log_dumper_test.cpp                    |  7 +++----
 modem/{ => dump_modemlog}/sepolicy/dump_modem.te |  0
 .../sepolicy/dump_modemlog.te                    |  0
 modem/{ => dump_modemlog}/sepolicy/file.te       |  0
 modem/{ => dump_modemlog}/sepolicy/file_contexts |  0
 .../{ => dump_modemlog}/sepolicy/genfs_contexts  |  0
 .../test/include/fake_android_property_manager.h | 12 +++++-------
 modem/modem.mk                                   |  6 +-----
 17 files changed, 42 insertions(+), 51 deletions(-)
 rename modem/{ => dump_modemlog}/Android.bp (65%)
 rename modem/{ => dump_modemlog}/dump_modem.sh (100%)
 rename modem/{ => dump_modemlog}/dump_modemlog.cpp (89%)
 create mode 100644 modem/dump_modemlog/dump_modemlog.mk
 rename modem/{ => dump_modemlog}/include/android_property_manager.h (86%)
 rename modem/{ => dump_modemlog}/include/dumper.h (96%)
 rename modem/{ => dump_modemlog}/include/modem_log_constants.h (96%)
 rename modem/{ => dump_modemlog}/include/modem_log_dumper.h (96%)
 rename modem/{ => dump_modemlog}/modem_log_dumper.cpp (96%)
 rename modem/{test => dump_modemlog}/modem_log_dumper_test.cpp (97%)
 rename modem/{ => dump_modemlog}/sepolicy/dump_modem.te (100%)
 rename modem/{ => dump_modemlog}/sepolicy/dump_modemlog.te (100%)
 rename modem/{ => dump_modemlog}/sepolicy/file.te (100%)
 rename modem/{ => dump_modemlog}/sepolicy/file_contexts (100%)
 rename modem/{ => dump_modemlog}/sepolicy/genfs_contexts (100%)
 rename modem/{ => dump_modemlog}/test/include/fake_android_property_manager.h (91%)

diff --git a/modem/Android.bp b/modem/dump_modemlog/Android.bp
similarity index 65%
rename from modem/Android.bp
rename to modem/dump_modemlog/Android.bp
index dbc1cac..b264609 100644
--- a/modem/Android.bp
+++ b/modem/dump_modemlog/Android.bp
@@ -11,9 +11,9 @@ sh_binary {
 
 cc_defaults {
     name: "dump_modemlog_defaults",
-    srcs: ["modem_log_dumper.cpp"],
-    local_include_dirs: ["include"],
-    shared_libs: ["liblog"],
+    srcs: [ "modem_log_dumper.cpp" ],
+    local_include_dirs: [ "include" ],
+    shared_libs: [ "liblog" ],
 }
 
 cc_binary {
@@ -29,16 +29,16 @@ cc_binary {
         "libdump",
         "liblog",
     ],
-    defaults: ["dump_modemlog_defaults"],
+    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"],
+    srcs: [ "*_test.cpp" ],
+    defaults: [ "dump_modemlog_defaults" ],
+    local_include_dirs: [ "test/include" ],
+    static_libs: [ "libgmock" ],
     vendor: true,
 }
diff --git a/modem/dump_modem.sh b/modem/dump_modemlog/dump_modem.sh
similarity index 100%
rename from modem/dump_modem.sh
rename to modem/dump_modemlog/dump_modem.sh
diff --git a/modem/dump_modemlog.cpp b/modem/dump_modemlog/dump_modemlog.cpp
similarity index 89%
rename from modem/dump_modemlog.cpp
rename to modem/dump_modemlog/dump_modemlog.cpp
index 1b6b2e9..47181cb 100644
--- a/modem/dump_modemlog.cpp
+++ b/modem/dump_modemlog/dump_modemlog.cpp
@@ -19,8 +19,7 @@
 #include "dumper.h"
 #include "modem_log_dumper.h"
 
-namespace modem {
-namespace logging {
+namespace pixel_modem::logging {
 
 /**
  * @brief Implementation of AndroidPropertyManager that directly forwards to
@@ -59,13 +58,13 @@ class DumperImpl : public Dumper {
   }
 };
 
-}  // namespace logging
-}  // namespace modem
+}  // namespace pixel_modem::logging
 
 int main() {
-  modem::logging::DumperImpl dumper_impl;
-  modem::logging::AndroidPropertyManagerImpl android_property_manager_impl;
-  modem::logging::ModemLogDumper modem_log_dumper(
+  pixel_modem::logging::DumperImpl dumper_impl;
+  pixel_modem::logging::AndroidPropertyManagerImpl
+      android_property_manager_impl;
+  pixel_modem::logging::ModemLogDumper modem_log_dumper(
       dumper_impl, android_property_manager_impl);
 
   modem_log_dumper.DumpModemLogs();
diff --git a/modem/dump_modemlog/dump_modemlog.mk b/modem/dump_modemlog/dump_modemlog.mk
new file mode 100644
index 0000000..5e91ab7
--- /dev/null
+++ b/modem/dump_modemlog/dump_modemlog.mk
@@ -0,0 +1,5 @@
+BOARD_VENDOR_SEPOLICY_DIRS += device/google/gs-common/modem/dump_modemlog/sepolicy
+
+PRODUCT_PACKAGES += dump_modem.sh
+PRODUCT_PACKAGES += dump_modemlog
+
diff --git a/modem/include/android_property_manager.h b/modem/dump_modemlog/include/android_property_manager.h
similarity index 86%
rename from modem/include/android_property_manager.h
rename to modem/dump_modemlog/include/android_property_manager.h
index 7135d66..eb426c3 100644
--- a/modem/include/android_property_manager.h
+++ b/modem/dump_modemlog/include/android_property_manager.h
@@ -2,8 +2,7 @@
 
 #include <string>
 
-namespace modem {
-namespace logging {
+namespace pixel_modem::logging {
 
 /**
  * @brief Interface for interacting with Android System Properties.
@@ -17,5 +16,5 @@ class AndroidPropertyManager {
   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
+
+}  // namespace pixel_modem::logging
diff --git a/modem/include/dumper.h b/modem/dump_modemlog/include/dumper.h
similarity index 96%
rename from modem/include/dumper.h
rename to modem/dump_modemlog/include/dumper.h
index 348e666..a9b96c6 100644
--- a/modem/include/dumper.h
+++ b/modem/dump_modemlog/include/dumper.h
@@ -3,8 +3,7 @@
 #include <ostream>
 #include <string_view>
 
-namespace modem {
-namespace logging {
+namespace pixel_modem::logging {
 
 /**
  * @brief Data object for information about dumpings logs.
@@ -67,5 +66,5 @@ class Dumper {
   virtual void DumpLogs(const LogDumpInfo& log_dump_info);
   virtual void CopyFile(const FileCopyInfo& file_copy_info);
 };
-}  // namespace logging
-}  // namespace modem
+
+}  // namespace pixel_modem::logging
diff --git a/modem/include/modem_log_constants.h b/modem/dump_modemlog/include/modem_log_constants.h
similarity index 96%
rename from modem/include/modem_log_constants.h
rename to modem/dump_modemlog/include/modem_log_constants.h
index 29a0fa8..8183ec3 100644
--- a/modem/include/modem_log_constants.h
+++ b/modem/dump_modemlog/include/modem_log_constants.h
@@ -3,8 +3,7 @@
 
 #include "dumper.h"
 
-namespace modem {
-namespace logging {
+namespace pixel_modem::logging {
 
 // Modem related Android System Properties
 
@@ -52,5 +51,5 @@ constexpr static FileCopyInfo kFileCopyInfo[] = {
     {.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
+
+}  // namespace pixel_modem::logging
diff --git a/modem/include/modem_log_dumper.h b/modem/dump_modemlog/include/modem_log_dumper.h
similarity index 96%
rename from modem/include/modem_log_dumper.h
rename to modem/dump_modemlog/include/modem_log_dumper.h
index 96911b0..1533217 100644
--- a/modem/include/modem_log_dumper.h
+++ b/modem/dump_modemlog/include/modem_log_dumper.h
@@ -3,8 +3,7 @@
 #include "android_property_manager.h"
 #include "dumper.h"
 
-namespace modem {
-namespace logging {
+namespace pixel_modem::logging {
 
 /**
  * @brief Responsible for dumping all relevant modem logs.
@@ -77,5 +76,4 @@ class ModemLogDumper {
   AndroidPropertyManager& android_property_manager_;
 };
 
-}  // namespace logging
-}  // namespace modem
+}  // namespace pixel_modem::logging
diff --git a/modem/modem_log_dumper.cpp b/modem/dump_modemlog/modem_log_dumper.cpp
similarity index 96%
rename from modem/modem_log_dumper.cpp
rename to modem/dump_modemlog/modem_log_dumper.cpp
index fad8d29..b5e7a07 100644
--- a/modem/modem_log_dumper.cpp
+++ b/modem/dump_modemlog/modem_log_dumper.cpp
@@ -5,8 +5,7 @@
 #include "dumper.h"
 #include "modem_log_constants.h"
 
-namespace modem {
-namespace logging {
+namespace pixel_modem::logging {
 
 void ModemLogDumper::DumpModemLogs() {
   bool shouldRestartModemLogging =
@@ -76,5 +75,5 @@ void ModemLogDumper::startModemLogging() {
   android_property_manager_.SetProperty(kModemLoggingEnabledProperty.data(),
                                         "true");
 }
-}  // namespace logging
-}  // namespace modem
+
+}  // namespace pixel_modem::logging
diff --git a/modem/test/modem_log_dumper_test.cpp b/modem/dump_modemlog/modem_log_dumper_test.cpp
similarity index 97%
rename from modem/test/modem_log_dumper_test.cpp
rename to modem/dump_modemlog/modem_log_dumper_test.cpp
index a052d43..d9917e1 100644
--- a/modem/test/modem_log_dumper_test.cpp
+++ b/modem/dump_modemlog/modem_log_dumper_test.cpp
@@ -7,8 +7,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
-namespace modem {
-namespace logging {
+namespace pixel_modem::logging {
 namespace {
 
 using ::testing::Eq;
@@ -101,6 +100,6 @@ TEST_F(ModemLogDumperTest,
 
   EXPECT_FALSE(fake_android_property_manager.ModemLoggingHasRestarted());
 }
+
 }  // namespace
-}  // namespace logging
-}  // namespace modem
+}  // namespace pixel_modem::logging
diff --git a/modem/sepolicy/dump_modem.te b/modem/dump_modemlog/sepolicy/dump_modem.te
similarity index 100%
rename from modem/sepolicy/dump_modem.te
rename to modem/dump_modemlog/sepolicy/dump_modem.te
diff --git a/modem/sepolicy/dump_modemlog.te b/modem/dump_modemlog/sepolicy/dump_modemlog.te
similarity index 100%
rename from modem/sepolicy/dump_modemlog.te
rename to modem/dump_modemlog/sepolicy/dump_modemlog.te
diff --git a/modem/sepolicy/file.te b/modem/dump_modemlog/sepolicy/file.te
similarity index 100%
rename from modem/sepolicy/file.te
rename to modem/dump_modemlog/sepolicy/file.te
diff --git a/modem/sepolicy/file_contexts b/modem/dump_modemlog/sepolicy/file_contexts
similarity index 100%
rename from modem/sepolicy/file_contexts
rename to modem/dump_modemlog/sepolicy/file_contexts
diff --git a/modem/sepolicy/genfs_contexts b/modem/dump_modemlog/sepolicy/genfs_contexts
similarity index 100%
rename from modem/sepolicy/genfs_contexts
rename to modem/dump_modemlog/sepolicy/genfs_contexts
diff --git a/modem/test/include/fake_android_property_manager.h b/modem/dump_modemlog/test/include/fake_android_property_manager.h
similarity index 91%
rename from modem/test/include/fake_android_property_manager.h
rename to modem/dump_modemlog/test/include/fake_android_property_manager.h
index 79fd4f1..0d5731a 100644
--- a/modem/test/include/fake_android_property_manager.h
+++ b/modem/dump_modemlog/test/include/fake_android_property_manager.h
@@ -7,8 +7,7 @@
 #include "android_property_manager.h"
 #include "modem_log_constants.h"
 
-namespace modem {
-namespace logging {
+namespace pixel_modem::logging {
 
 /**
  * @brief Fake Implementation of AndroidPropertyManager that mocks some of the
@@ -20,9 +19,8 @@ class FakeAndroidPropertyManager : public AndroidPropertyManager {
   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;
+    return MapContainsKey(key) ? GetPropertyInternal(key) == kTruthString
+                               : default_value;
   };
 
   std::string GetProperty(const std::string& key,
@@ -73,5 +71,5 @@ class FakeAndroidPropertyManager : public AndroidPropertyManager {
   bool modem_logging_has_been_off_ = false;
   bool modem_logging_has_restarted_ = false;
 };
-}  // namespace logging
-}  // namespace modem
+
+}  // namespace pixel_modem::logging
diff --git a/modem/modem.mk b/modem/modem.mk
index 10df7d4..d921e74 100644
--- a/modem/modem.mk
+++ b/modem/modem.mk
@@ -1,5 +1 @@
-BOARD_VENDOR_SEPOLICY_DIRS += device/google/gs-common/modem/sepolicy
-
-PRODUCT_PACKAGES += dump_modem.sh
-PRODUCT_PACKAGES += dump_modemlog
-
+include device/google/gs-common/modem/dump_modemlog/dump_modemlog.mk

From 047f0aca49217d58b4af8c8ac8652585d5b29eb3 Mon Sep 17 00:00:00 2001
From: kierancyphus <kierancyphus@google.com>
Date: Fri, 10 Nov 2023 15:02:57 +0800
Subject: [PATCH 2/6] dump_modemlog: move android_property_manager

android_property_manager is moved to its own folder in the root of the
modem folder. This is so that libeomservice proxy has a specific build
target to include.

Test: build, flash, check modem logs in bugreport
Bug: 302435001
Change-Id: Ifc4a0c888717f5c28cf9b642d0b978b495be29d0
---
 modem/android_property_manager/Android.bp     |  9 +++
 .../android_property_manager/fake/Android.bp  | 23 ++++++
 .../fake/fake_android_property_manager.cpp    | 80 +++++++++++++++++++
 .../include/fake_android_property_manager.h   | 54 +++++++++++++
 .../android_property_manager/impl/Android.bp  | 18 +++++
 .../impl/android_property_manager_impl.cpp    | 29 +++++++
 .../include/android_property_manager_impl.h   | 25 ++++++
 .../include/android_property_manager.h        | 19 +++--
 modem/dump_modemlog/Android.bp                | 54 ++++++++++---
 modem/dump_modemlog/dump_modemlog.cpp         | 26 +-----
 .../include/fake_android_property_manager.h   | 75 -----------------
 11 files changed, 296 insertions(+), 116 deletions(-)
 create mode 100644 modem/android_property_manager/Android.bp
 create mode 100644 modem/android_property_manager/fake/Android.bp
 create mode 100644 modem/android_property_manager/fake/fake_android_property_manager.cpp
 create mode 100644 modem/android_property_manager/fake/include/fake_android_property_manager.h
 create mode 100644 modem/android_property_manager/impl/Android.bp
 create mode 100644 modem/android_property_manager/impl/android_property_manager_impl.cpp
 create mode 100644 modem/android_property_manager/impl/include/android_property_manager_impl.h
 rename modem/{dump_modemlog => android_property_manager}/include/android_property_manager.h (53%)
 delete mode 100644 modem/dump_modemlog/test/include/fake_android_property_manager.h

diff --git a/modem/android_property_manager/Android.bp b/modem/android_property_manager/Android.bp
new file mode 100644
index 0000000..af89aec
--- /dev/null
+++ b/modem/android_property_manager/Android.bp
@@ -0,0 +1,9 @@
+package {
+    default_applicable_licenses: ["Android-Apache-2.0"],
+}
+
+cc_library {
+    name: "modem_android_property_manager",
+    export_include_dirs: [ "include" ],
+    vendor_available: true,
+}
diff --git a/modem/android_property_manager/fake/Android.bp b/modem/android_property_manager/fake/Android.bp
new file mode 100644
index 0000000..247b97c
--- /dev/null
+++ b/modem/android_property_manager/fake/Android.bp
@@ -0,0 +1,23 @@
+package {
+    default_applicable_licenses: ["Android-Apache-2.0"],
+}
+
+// When `modem_android_property_manager_fake` is included statically, its
+// dependencies are not transitively included, so the target will also have to
+// include this default to restate them.
+cc_defaults {
+    name: "modem_android_property_manager_fake_defaults",
+    static_libs: [
+        "modem_android_property_manager",
+        "libbase",
+        "modem_log_constants",
+    ],
+}
+
+cc_library_static {
+    name: "modem_android_property_manager_fake",
+    export_include_dirs: [ "include" ],
+    srcs: [ "fake_android_property_manager.cpp" ],
+    defaults: [ "modem_android_property_manager_fake_defaults" ],
+    vendor_available: true,
+}
diff --git a/modem/android_property_manager/fake/fake_android_property_manager.cpp b/modem/android_property_manager/fake/fake_android_property_manager.cpp
new file mode 100644
index 0000000..d25d6da
--- /dev/null
+++ b/modem/android_property_manager/fake/fake_android_property_manager.cpp
@@ -0,0 +1,80 @@
+#include "fake_android_property_manager.h"
+
+#include <android-base/parseint.h>
+#include <android-base/result.h>
+
+#include <cerrno>
+#include <map>
+#include <string>
+
+#include "modem_log_constants.h"
+
+namespace pixel_modem {
+
+bool FakeAndroidPropertyManager::GetBoolProperty(const std::string& key,
+                                                 bool default_value) {
+  auto value_result = GetProperty(key);
+  return value_result.ok() ? (*value_result) == kTruthString : default_value;
+}
+
+std::string FakeAndroidPropertyManager::GetProperty(
+    const std::string& key, const std::string& default_value) {
+  auto value_result = GetProperty(key);
+  return value_result.ok() ? *value_result : default_value;
+}
+
+int FakeAndroidPropertyManager::GetIntProperty(const std::string& key,
+                                               int default_value) {
+  int value = default_value;
+
+  auto property_result = GetProperty(key);
+  if (property_result.ok()) {
+    android::base::ParseInt<int>((*property_result).data(), &value);
+  }
+  return 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.
+ */
+bool FakeAndroidPropertyManager::SetProperty(const std::string& key,
+                                             const std::string& value) {
+  if (key == logging::kModemLoggingEnabledProperty) {
+    property_map_[logging::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;
+  return true;
+}
+
+/**
+ * @brief Gets android system property if present.
+ *
+ * @param[in] key Name of property.
+ *
+ * @return Status of get operation and value if successful.
+ * @retval EINVAL Key not present in map.
+ */
+android::base::Result<std::string> FakeAndroidPropertyManager::GetProperty(
+    const std::string& key) {
+  const auto it = property_map_.find(key);
+  if (it == property_map_.end()) {
+    return android::base::Error()
+           << "Property: " << key << " not found." << EINVAL;
+  }
+  return it->second;
+}
+
+}  // namespace pixel_modem
diff --git a/modem/android_property_manager/fake/include/fake_android_property_manager.h b/modem/android_property_manager/fake/include/fake_android_property_manager.h
new file mode 100644
index 0000000..eeea5a0
--- /dev/null
+++ b/modem/android_property_manager/fake/include/fake_android_property_manager.h
@@ -0,0 +1,54 @@
+#pragma once
+
+#include <map>
+#include <string>
+
+#include "android-base/result.h"
+#include "android_property_manager.h"
+
+namespace pixel_modem {
+
+/**
+ * @brief Fake Implementation of AndroidPropertyManager that mocks some of the
+ * property changing behaviour from pixellogger's `modem_logging_control`.
+ */
+class FakeAndroidPropertyManager : public AndroidPropertyManager {
+ public:
+  bool GetBoolProperty(const std::string& key, bool default_value) override;
+
+  std::string GetProperty(const std::string& key,
+                          const std::string& default_value) override;
+
+  int GetIntProperty(const std::string& key, int default_value) override;
+
+  /**
+   * 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.
+   */
+  bool SetProperty(const std::string& key, const std::string& value) override;
+
+  inline bool ModemLoggingHasRestarted() {
+    return modem_logging_has_restarted_;
+  }
+
+ private:
+  /**
+   * @brief Gets android system property if present.
+   *
+   * @param[in] key Name of property.
+   *
+   * @return Status of get operation and value if successful.
+   * @retval EINVAL Key not present in map.
+   */
+  android::base::Result<std::string> GetProperty(const std::string& key);
+
+  std::map<std::string, std::string> property_map_;
+  bool modem_logging_has_been_off_ = false;
+  bool modem_logging_has_restarted_ = false;
+};
+
+}  // namespace pixel_modem
diff --git a/modem/android_property_manager/impl/Android.bp b/modem/android_property_manager/impl/Android.bp
new file mode 100644
index 0000000..2023e8f
--- /dev/null
+++ b/modem/android_property_manager/impl/Android.bp
@@ -0,0 +1,18 @@
+package {
+    default_applicable_licenses: ["Android-Apache-2.0"],
+}
+
+modem_android_property_manager_impl_public_deps = [
+    "modem_android_property_manager",
+]
+
+cc_library_shared {
+    name: "modem_android_property_manager_impl",
+    export_include_dirs: [ "include" ],
+    srcs: [ "android_property_manager_impl.cpp" ],
+    shared_libs: modem_android_property_manager_impl_public_deps + [
+        "libbase",
+    ],
+    export_shared_lib_headers: modem_android_property_manager_impl_public_deps,
+    vendor_available: true,
+}
diff --git a/modem/android_property_manager/impl/android_property_manager_impl.cpp b/modem/android_property_manager/impl/android_property_manager_impl.cpp
new file mode 100644
index 0000000..b6b900a
--- /dev/null
+++ b/modem/android_property_manager/impl/android_property_manager_impl.cpp
@@ -0,0 +1,29 @@
+#include "android_property_manager_impl.h"
+
+#include <android-base/properties.h>
+
+#include <string>
+
+namespace pixel_modem {
+
+bool AndroidPropertyManagerImpl::GetBoolProperty(const std::string& key,
+                                                 bool default_value) {
+  return android::base::GetBoolProperty(key, default_value);
+}
+
+std::string AndroidPropertyManagerImpl::GetProperty(
+    const std::string& key, const std::string& default_value) {
+  return android::base::GetProperty(key, default_value);
+}
+
+int AndroidPropertyManagerImpl::GetIntProperty(const std::string& key,
+                                               int default_value) {
+  return android::base::GetIntProperty(key, default_value);
+}
+
+bool AndroidPropertyManagerImpl::SetProperty(const std::string& key,
+                                             const std::string& value) {
+  return android::base::SetProperty(key, value);
+}
+
+}  // namespace pixel_modem
diff --git a/modem/android_property_manager/impl/include/android_property_manager_impl.h b/modem/android_property_manager/impl/include/android_property_manager_impl.h
new file mode 100644
index 0000000..91cee25
--- /dev/null
+++ b/modem/android_property_manager/impl/include/android_property_manager_impl.h
@@ -0,0 +1,25 @@
+#pragma once
+
+#include <string>
+
+#include "android_property_manager.h"
+
+namespace pixel_modem {
+
+/**
+ * @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;
+
+  std::string GetProperty(const std::string& key,
+                          const std::string& default_value) override;
+
+  int GetIntProperty(const std::string& key, int default_value) override;
+
+  bool SetProperty(const std::string& key, const std::string& value) override;
+};
+
+}  // namespace pixel_modem
diff --git a/modem/dump_modemlog/include/android_property_manager.h b/modem/android_property_manager/include/android_property_manager.h
similarity index 53%
rename from modem/dump_modemlog/include/android_property_manager.h
rename to modem/android_property_manager/include/android_property_manager.h
index eb426c3..e41a28d 100644
--- a/modem/dump_modemlog/include/android_property_manager.h
+++ b/modem/android_property_manager/include/android_property_manager.h
@@ -1,8 +1,14 @@
+
 #pragma once
 
 #include <string>
+#include <string_view>
 
-namespace pixel_modem::logging {
+namespace pixel_modem {
+
+// Used to set boolean parameters to true / false
+inline constexpr std::string_view kTruthString = "true";
+inline constexpr std::string_view kFalseString = "false";
 
 /**
  * @brief Interface for interacting with Android System Properties.
@@ -10,11 +16,12 @@ namespace pixel_modem::logging {
 class AndroidPropertyManager {
  public:
   virtual ~AndroidPropertyManager() = default;
-  virtual bool GetBoolProperty(const std::string& key, bool default_value);
+  virtual bool GetBoolProperty(const std::string& key, bool default_value) = 0;
   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);
+                                  const std::string& default_value) = 0;
+  virtual int GetIntProperty(const std::string& key, int default_value) = 0;
+  virtual bool SetProperty(const std::string& key,
+                           const std::string& value) = 0;
 };
 
-}  // namespace pixel_modem::logging
+}  // namespace pixel_modem
diff --git a/modem/dump_modemlog/Android.bp b/modem/dump_modemlog/Android.bp
index b264609..cad6e49 100644
--- a/modem/dump_modemlog/Android.bp
+++ b/modem/dump_modemlog/Android.bp
@@ -1,5 +1,5 @@
 package {
-    default_applicable_licenses: ["Android-Apache-2.0"],
+    default_applicable_licenses: [ "Android-Apache-2.0" ],
 }
 
 sh_binary {
@@ -9,16 +9,38 @@ sh_binary {
     sub_dir: "dump",
 }
 
+// Modem Log Dumper
+
+modem_log_dumper_public_deps = [
+    "modem_android_property_manager",
+]
+
+// When `modem_log_dumper` is included statically, its dependencies are not
+// transitively included, so the target will also have to include this default
+// to restate them.
 cc_defaults {
-    name: "dump_modemlog_defaults",
-    srcs: [ "modem_log_dumper.cpp" ],
-    local_include_dirs: [ "include" ],
-    shared_libs: [ "liblog" ],
+    name: "modem_log_dumper_defaults",
+    shared_libs: modem_log_dumper_public_deps + [
+        "libbase",
+        // liblog is not directly used by us, but it's a transitive dependency of libbase
+        "liblog",
+    ],
 }
 
+cc_library {
+    name: "modem_log_dumper",
+    srcs: [ "modem_log_dumper.cpp" ],
+    defaults: [ "modem_log_dumper_defaults" ],
+    export_shared_lib_headers: modem_log_dumper_public_deps,
+    export_include_dirs: [ "include" ],
+    vendor_available: true,
+}
+
+// dump_modemlog
+
 cc_binary {
     name: "dump_modemlog",
-    srcs: ["dump_modemlog.cpp"],
+    srcs: [ "dump_modemlog.cpp" ],
     cflags: [
         "-Wall",
         "-Wextra",
@@ -28,17 +50,27 @@ cc_binary {
         "libbase",
         "libdump",
         "liblog",
+        "modem_android_property_manager_impl",
+        "modem_log_dumper",
     ],
-    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" ],
+    srcs: [ "modem_log_dumper_test.cpp" ],
+    defaults: [
+        "modem_log_dumper_defaults",
+        "modem_android_property_manager_fake_defaults",
+    ],
+    static_libs: [
+        "modem_log_dumper",
+        "modem_android_property_manager_fake",
+        "libgmock",
+    ],
     vendor: true,
+    // Shared libs in vendor folder are guarded by SEPolicy, so tests need root
+    // access to run them.
+    require_root: true,
 }
diff --git a/modem/dump_modemlog/dump_modemlog.cpp b/modem/dump_modemlog/dump_modemlog.cpp
index 47181cb..bed936c 100644
--- a/modem/dump_modemlog/dump_modemlog.cpp
+++ b/modem/dump_modemlog/dump_modemlog.cpp
@@ -16,33 +16,12 @@
 #include <android-base/properties.h>
 #include <dump/pixel_dump.h>
 
+#include "android_property_manager_impl.h"
 #include "dumper.h"
 #include "modem_log_dumper.h"
 
 namespace pixel_modem::logging {
 
-/**
- * @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.
@@ -62,8 +41,7 @@ class DumperImpl : public Dumper {
 
 int main() {
   pixel_modem::logging::DumperImpl dumper_impl;
-  pixel_modem::logging::AndroidPropertyManagerImpl
-      android_property_manager_impl;
+  pixel_modem::AndroidPropertyManagerImpl android_property_manager_impl;
   pixel_modem::logging::ModemLogDumper modem_log_dumper(
       dumper_impl, android_property_manager_impl);
 
diff --git a/modem/dump_modemlog/test/include/fake_android_property_manager.h b/modem/dump_modemlog/test/include/fake_android_property_manager.h
deleted file mode 100644
index 0d5731a..0000000
--- a/modem/dump_modemlog/test/include/fake_android_property_manager.h
+++ /dev/null
@@ -1,75 +0,0 @@
-
-
-#include <map>
-#include <string>
-#include <string_view>
-
-#include "android_property_manager.h"
-#include "modem_log_constants.h"
-
-namespace pixel_modem::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 pixel_modem::logging

From 0944a8db529a01a9a7a770b1dd2833311ec8f212 Mon Sep 17 00:00:00 2001
From: kierancyphus <kierancyphus@google.com>
Date: Fri, 10 Nov 2023 15:25:02 +0800
Subject: [PATCH 3/6] gs-common/modem: clock manager interface

A lot of modem code requires sleeping while vendor services do some
background processing. Since we don't want to actually sleep for unit
tests, an interface is provided here so that a fake sleep can be
injected.

Test: N/A. Directly forwards methods or does nothing.
Bug: 302435001
Change-Id: I3bcf0307156d93756d69cd9f749c88b508ba9466
---
 modem/clock_manager/Android.bp                |  9 ++++++
 modem/clock_manager/fake/Android.bp           | 15 ++++++++++
 .../fake/include/fake_clock_manager.h         | 21 ++++++++++++++
 modem/clock_manager/impl/Android.bp           | 16 +++++++++++
 .../clock_manager/impl/clock_manager_impl.cpp |  9 ++++++
 .../impl/include/clock_manager_impl.h         | 13 +++++++++
 modem/clock_manager/include/clock_manager.h   | 28 +++++++++++++++++++
 7 files changed, 111 insertions(+)
 create mode 100644 modem/clock_manager/Android.bp
 create mode 100644 modem/clock_manager/fake/Android.bp
 create mode 100644 modem/clock_manager/fake/include/fake_clock_manager.h
 create mode 100644 modem/clock_manager/impl/Android.bp
 create mode 100644 modem/clock_manager/impl/clock_manager_impl.cpp
 create mode 100644 modem/clock_manager/impl/include/clock_manager_impl.h
 create mode 100644 modem/clock_manager/include/clock_manager.h

diff --git a/modem/clock_manager/Android.bp b/modem/clock_manager/Android.bp
new file mode 100644
index 0000000..98aff6f
--- /dev/null
+++ b/modem/clock_manager/Android.bp
@@ -0,0 +1,9 @@
+package {
+    default_applicable_licenses: ["Android-Apache-2.0"],
+}
+
+cc_library {
+    name: "modem_clock_manager",
+    export_include_dirs: [ "include" ],
+    vendor_available: true,
+}
diff --git a/modem/clock_manager/fake/Android.bp b/modem/clock_manager/fake/Android.bp
new file mode 100644
index 0000000..eb16445
--- /dev/null
+++ b/modem/clock_manager/fake/Android.bp
@@ -0,0 +1,15 @@
+package {
+    default_applicable_licenses: ["Android-Apache-2.0"],
+}
+
+cc_defaults {
+    name: "fake_modem_clock_manager_defaults",
+    shared_libs: [ "modem_clock_manager" ],
+}
+
+cc_library_static {
+    name: "fake_modem_clock_manager",
+    export_include_dirs: [ "include" ],
+    defaults: [ "fake_modem_clock_manager_defaults" ],
+    vendor_available: true,
+}
diff --git a/modem/clock_manager/fake/include/fake_clock_manager.h b/modem/clock_manager/fake/include/fake_clock_manager.h
new file mode 100644
index 0000000..8956777
--- /dev/null
+++ b/modem/clock_manager/fake/include/fake_clock_manager.h
@@ -0,0 +1,21 @@
+#include "clock_manager.h"
+
+namespace pixel_modem {
+
+/**
+ * @brief Fake implementation of clock manager that doesn't actually sleep.
+ *
+ * A lot of vendor code don't have return values and instead force the client
+ * codes to sleep for a specified period of time before checking some system
+ * properties. However, since unit tests don't rely on the real vendor
+ * implementations, these sleeps should be ignored and so a fake clock should be
+ * used.
+ *
+ * Since this definition is unlikely to change, it will be defined in the header
+ * and not an implementation file.
+ */
+struct FakeClockManager : public ClockManager {
+  void Sleep(size_t /*seconds*/) const override{};
+};
+
+}  // namespace pixel_modem
diff --git a/modem/clock_manager/impl/Android.bp b/modem/clock_manager/impl/Android.bp
new file mode 100644
index 0000000..13f4cc6
--- /dev/null
+++ b/modem/clock_manager/impl/Android.bp
@@ -0,0 +1,16 @@
+package {
+    default_applicable_licenses: ["Android-Apache-2.0"],
+}
+
+modem_clock_manager_impl_public_deps = [
+    "modem_clock_manager",
+]
+
+cc_library {
+    name: "modem_clock_manager_impl",
+    export_include_dirs: [ "include" ],
+    srcs: [ "clock_manager_impl.cpp" ],
+    shared_libs: modem_clock_manager_impl_public_deps,
+    export_shared_lib_headers: modem_clock_manager_impl_public_deps,
+    vendor_available: true,
+}
diff --git a/modem/clock_manager/impl/clock_manager_impl.cpp b/modem/clock_manager/impl/clock_manager_impl.cpp
new file mode 100644
index 0000000..dc61a63
--- /dev/null
+++ b/modem/clock_manager/impl/clock_manager_impl.cpp
@@ -0,0 +1,9 @@
+#include "clock_manager_impl.h"
+
+#include <unistd.h>
+
+namespace pixel_modem {
+
+void ClockManagerImpl::Sleep(size_t seconds) const { sleep(seconds); }
+
+}  // namespace pixel_modem
diff --git a/modem/clock_manager/impl/include/clock_manager_impl.h b/modem/clock_manager/impl/include/clock_manager_impl.h
new file mode 100644
index 0000000..d626b98
--- /dev/null
+++ b/modem/clock_manager/impl/include/clock_manager_impl.h
@@ -0,0 +1,13 @@
+#pragma once
+
+#include <cstddef>
+
+#include "clock_manager.h"
+
+namespace pixel_modem {
+
+struct ClockManagerImpl : public ClockManager {
+  void Sleep(size_t seconds) const override;
+};
+
+}  // namespace pixel_modem
diff --git a/modem/clock_manager/include/clock_manager.h b/modem/clock_manager/include/clock_manager.h
new file mode 100644
index 0000000..1db88c5
--- /dev/null
+++ b/modem/clock_manager/include/clock_manager.h
@@ -0,0 +1,28 @@
+#pragma once
+
+#include <cstddef>
+
+namespace pixel_modem {
+
+/**
+ * @brief Interface for time based operations.
+ *
+ * This interface was intentionally not called `Clock`, like the Java side
+ * counterpart since it's likely that clients would call the local variable
+ * `clock(_)`, which would clash with the C defined `clock` method.
+ */
+struct ClockManager {
+  virtual ~ClockManager() = default;
+
+  /**
+   * @brief Sleep the thread for a given number of seconds.
+   *
+   * @param seconds Minimum number of seconds to sleep for. Note, this is
+   * different than the Java android clock which accepts seconds. This was done
+   * because C++ developers are likely more familiar with the `sleep` command,
+   * which accepts seconds.
+   */
+  virtual void Sleep(size_t seconds) const = 0;
+};
+
+}  // namespace pixel_modem

From 93c22b6672a3b19a58ccf9692b45160130fea7d3 Mon Sep 17 00:00:00 2001
From: kierancyphus <kierancyphus@google.com>
Date: Fri, 10 Nov 2023 15:32:26 +0800
Subject: [PATCH 4/6] modem/modem_log_constants: create common folder

A lot of different modem related processes require reading / writing to
the same android system properties. This CL solifies them all into
one place to avoid duplication.

Test: build
Bug: 302435001
Change-Id: I113f43bb68833224f45ad91668cd327587e1649b
---
 modem/dump_modemlog/Android.bp                |  5 +-
 .../include/bugreport_constants.h             | 33 +++++++++++
 .../include/modem_log_constants.h             | 55 -------------------
 modem/dump_modemlog/modem_log_dumper.cpp      |  1 +
 modem/dump_modemlog/modem_log_dumper_test.cpp |  8 ++-
 modem/modem_log_constants/Android.bp          |  9 +++
 .../include/modem_log_constants.h             | 31 +++++++++++
 7 files changed, 83 insertions(+), 59 deletions(-)
 create mode 100644 modem/dump_modemlog/include/bugreport_constants.h
 delete mode 100644 modem/dump_modemlog/include/modem_log_constants.h
 create mode 100644 modem/modem_log_constants/Android.bp
 create mode 100644 modem/modem_log_constants/include/modem_log_constants.h

diff --git a/modem/dump_modemlog/Android.bp b/modem/dump_modemlog/Android.bp
index cad6e49..aca7b20 100644
--- a/modem/dump_modemlog/Android.bp
+++ b/modem/dump_modemlog/Android.bp
@@ -24,6 +24,7 @@ cc_defaults {
         "libbase",
         // liblog is not directly used by us, but it's a transitive dependency of libbase
         "liblog",
+        "modem_log_constants",
     ],
 }
 
@@ -65,8 +66,10 @@ cc_test {
         "modem_android_property_manager_fake_defaults",
     ],
     static_libs: [
-        "modem_log_dumper",
+        "modem_android_property_manager",
         "modem_android_property_manager_fake",
+        "modem_log_constants",
+        "modem_log_dumper",
         "libgmock",
     ],
     vendor: true,
diff --git a/modem/dump_modemlog/include/bugreport_constants.h b/modem/dump_modemlog/include/bugreport_constants.h
new file mode 100644
index 0000000..25c800f
--- /dev/null
+++ b/modem/dump_modemlog/include/bugreport_constants.h
@@ -0,0 +1,33 @@
+#pragma once
+
+#include <string_view>
+
+#include "dumper.h"
+
+namespace pixel_modem::logging {
+
+inline constexpr std::string_view kBugreportPackingDirectory =
+    "/data/vendor/radio/logs/always-on/all_logs";
+
+inline constexpr 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 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 pixel_modem::logging
diff --git a/modem/dump_modemlog/include/modem_log_constants.h b/modem/dump_modemlog/include/modem_log_constants.h
deleted file mode 100644
index 8183ec3..0000000
--- a/modem/dump_modemlog/include/modem_log_constants.h
+++ /dev/null
@@ -1,55 +0,0 @@
-#pragma once
-#include <string>
-
-#include "dumper.h"
-
-namespace pixel_modem::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 pixel_modem::logging
diff --git a/modem/dump_modemlog/modem_log_dumper.cpp b/modem/dump_modemlog/modem_log_dumper.cpp
index b5e7a07..951f89f 100644
--- a/modem/dump_modemlog/modem_log_dumper.cpp
+++ b/modem/dump_modemlog/modem_log_dumper.cpp
@@ -2,6 +2,7 @@
 
 #include <log/log.h>
 
+#include "bugreport_constants.h"
 #include "dumper.h"
 #include "modem_log_constants.h"
 
diff --git a/modem/dump_modemlog/modem_log_dumper_test.cpp b/modem/dump_modemlog/modem_log_dumper_test.cpp
index d9917e1..81bec8b 100644
--- a/modem/dump_modemlog/modem_log_dumper_test.cpp
+++ b/modem/dump_modemlog/modem_log_dumper_test.cpp
@@ -2,10 +2,13 @@
 
 #include <string_view>
 
+#include "android_property_manager.h"
+#include "bugreport_constants.h"
 #include "dumper.h"
 #include "fake_android_property_manager.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include "modem_log_constants.h"
 
 namespace pixel_modem::logging {
 namespace {
@@ -22,9 +25,8 @@ inline constexpr static LogDumpInfo kAlwaysOnLogDumpInfo = {
 
 void StartModemLogging(
     FakeAndroidPropertyManager& fake_android_property_manager) {
-  fake_android_property_manager.SetProperty(
-      kModemLoggingEnabledProperty.data(),
-      FakeAndroidPropertyManager::kTruthString.data());
+  fake_android_property_manager.SetProperty(kModemLoggingEnabledProperty.data(),
+                                            kTruthString.data());
 }
 
 class MockDumper : public Dumper {
diff --git a/modem/modem_log_constants/Android.bp b/modem/modem_log_constants/Android.bp
new file mode 100644
index 0000000..f4e3177
--- /dev/null
+++ b/modem/modem_log_constants/Android.bp
@@ -0,0 +1,9 @@
+package {
+    default_applicable_licenses: ["Android-Apache-2.0"],
+}
+
+cc_library {
+    name: "modem_log_constants",
+    export_include_dirs: [ "include" ],
+    vendor_available: true,
+}
diff --git a/modem/modem_log_constants/include/modem_log_constants.h b/modem/modem_log_constants/include/modem_log_constants.h
new file mode 100644
index 0000000..133a2a2
--- /dev/null
+++ b/modem/modem_log_constants/include/modem_log_constants.h
@@ -0,0 +1,31 @@
+#pragma once
+
+#include <string_view>
+
+namespace pixel_modem::logging {
+
+// Modem related Android System Properties
+
+// Controls triggering `modem_logging_start` and `modem_logging_stop`.
+inline constexpr 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 std::string_view kModemLoggingStatusProperty =
+    "vendor.sys.modem.logging.status";
+// Int which specifies how many files to include in the bugreport.
+inline constexpr 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 std::string_view kModemLoggingPathProperty =
+    "vendor.sys.modem.logging.log_path";
+
+// Bugreport constants
+inline constexpr int kDefaultBugreportNumberFiles = 100;
+inline constexpr std::string_view kModemAlwaysOnLogDirectory =
+    "/data/vendor/radio/logs/always-on";
+inline constexpr std::string_view kModemLogPrefix = "sbuff_";
+
+}  // namespace pixel_modem::logging

From da3ebae5adab2a2b379d60d9d35a7de938eb4976 Mon Sep 17 00:00:00 2001
From: kierancyphus <kierancyphus@google.com>
Date: Mon, 13 Nov 2023 15:04:32 +0800
Subject: [PATCH 5/6] modem_log_constants: System props for logging

Several different services need to be able to set the output directory
for copying modem logs, as well as how many files should be copied.

Test: N/A just defining constants
Bug: 302435001
Change-Id: I3e9f2462a42e3b074810e6fb0a925a8ca026f89d
---
 modem/modem_log_constants/include/modem_log_constants.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/modem/modem_log_constants/include/modem_log_constants.h b/modem/modem_log_constants/include/modem_log_constants.h
index 133a2a2..68004cf 100644
--- a/modem/modem_log_constants/include/modem_log_constants.h
+++ b/modem/modem_log_constants/include/modem_log_constants.h
@@ -21,6 +21,10 @@ inline constexpr std::string_view kModemLoggingNumberBugreportProperty =
 // determine the logging type.
 inline constexpr std::string_view kModemLoggingPathProperty =
     "vendor.sys.modem.logging.log_path";
+inline constexpr std::string_view kModemLoggingLogCountProperty =
+    "vendor.sys.modem.logging.log_count";
+inline constexpr std::string_view kModemLoggingLogPath =
+    "vendor.sys.modem.logging.log_path";
 
 // Bugreport constants
 inline constexpr int kDefaultBugreportNumberFiles = 100;

From c7da8aa098f3a1675eba9567b5bf4f57d9a8b720 Mon Sep 17 00:00:00 2001
From: kierancyphus <kierancyphus@google.com>
Date: Wed, 15 Nov 2023 15:04:21 +0800
Subject: [PATCH 6/6] dump_modemlog: always move modem logs

Dynamic log mask events can occur without leaving the logging status
property as enabled, which means when dumpstate should always try to
stop modem logging so that the new logs can be copied over.

Having the copying of logs and stopping of modem logging combined in
one command is no longer an ideal design, so b/289435256 was created
to find a better solution to this.

Test: build, flash, trigger log mask event, check logs in bugreport.
Bug: 302435001
Change-Id: I56358d3f08ac1f2a6099ede14c5e17b5ebffabbd
---
 modem/dump_modemlog/modem_log_dumper.cpp | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/modem/dump_modemlog/modem_log_dumper.cpp b/modem/dump_modemlog/modem_log_dumper.cpp
index 951f89f..127478e 100644
--- a/modem/dump_modemlog/modem_log_dumper.cpp
+++ b/modem/dump_modemlog/modem_log_dumper.cpp
@@ -15,7 +15,11 @@ void ModemLogDumper::DumpModemLogs() {
       kModemLoggingNumberBugreportProperty.data(),
       kDefaultBugreportNumberFiles);
 
-  if (shouldRestartModemLogging) {
+  // Should always trigger `stopModemLogging`. This is because currently copying
+  // modem logs and stopping modem logging are entangled.
+  // TODO: b/289435256 - Always copy logs and return this to checking if logging
+  // is actively running.
+  if (allowedToStopModemLogging()) {
     // 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