From 52d632cbe7519a0efeb36363e775b1919a6d6c08 Mon Sep 17 00:00:00 2001 From: kierancyphus Date: Fri, 30 Jun 2023 04:50:50 +0800 Subject: [PATCH] dump_modemlog: include log mask history files in dumpstate This was missed in the porting over from gs201/.../dumpstate.cpp. Test: Manually trigger bugreport and ensure that LoggingHistory.csv and LoggingMaskHistory.csv are included Bug:284275049 Change-Id: Ia630f3f1883b338fa879cfd6ea6bdd4c2a00437c --- modem/Android.bp | 16 +++ modem/dump_modemlog.cpp | 95 +++++++++------- modem/include/android_property_manager.h | 21 ++++ modem/include/dumper.h | 71 ++++++++++++ modem/include/modem_log_constants.h | 56 +++++++++ modem/include/modem_log_dumper.h | 81 +++++++++++++ modem/modem_log_dumper.cpp | 80 +++++++++++++ .../include/fake_android_property_manager.h | 77 +++++++++++++ modem/test/modem_log_dumper_test.cpp | 106 ++++++++++++++++++ 9 files changed, 560 insertions(+), 43 deletions(-) create mode 100644 modem/include/android_property_manager.h create mode 100644 modem/include/dumper.h create mode 100644 modem/include/modem_log_constants.h create mode 100644 modem/include/modem_log_dumper.h create mode 100644 modem/modem_log_dumper.cpp create mode 100644 modem/test/include/fake_android_property_manager.h create mode 100644 modem/test/modem_log_dumper_test.cpp diff --git a/modem/Android.bp b/modem/Android.bp index 84bdd61..dbc1cac 100644 --- a/modem/Android.bp +++ b/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, +} diff --git a/modem/dump_modemlog.cpp b/modem/dump_modemlog.cpp index f7ef834..1b6b2e9 100644 --- a/modem/dump_modemlog.cpp +++ b/modem/dump_modemlog.cpp @@ -13,52 +13,61 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -#include #include -#include +#include -#define MODEM_LOGGING_PERSIST_PROPERTY "persist.vendor.sys.modem.logging.enable" -#define MODEM_LOGGING_PROPERTY "vendor.sys.modem.logging.enable" -#define MODEM_LOGGING_STATUS_PROPERTY "vendor.sys.modem.logging.status" -#define MODEM_LOGGING_NUMBER_BUGREPORT_PROPERTY "persist.vendor.sys.modem.logging.br_num" -#define MODEM_LOGGING_PATH_PROPERTY "vendor.sys.modem.logging.log_path" -#define MODEM_SIM_DIRECTORY "/data/vendor/radio/sim/" -#define MODEM_LOG_PREFIX "sbuff_" -#define SIM_POWERON_LOG_PREFIX "sim_poweron_log_" +#include "dumper.h" +#include "modem_log_dumper.h" + +namespace modem { +namespace 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. + */ +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); - if (modemLogEnabled && ::android::base::GetProperty(MODEM_LOGGING_PATH_PROPERTY, "") == MODEM_LOG_DIRECTORY) { - bool modemLogStarted = ::android::base::GetBoolProperty(MODEM_LOGGING_STATUS_PROPERTY, false); - int maxFileNum = ::android::base::GetIntProperty(MODEM_LOGGING_NUMBER_BUGREPORT_PROPERTY, 100); + modem::logging::DumperImpl dumper_impl; + modem::logging::AndroidPropertyManagerImpl android_property_manager_impl; + modem::logging::ModemLogDumper modem_log_dumper( + dumper_impl, android_property_manager_impl); - if (modemLogStarted) { - ::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_log_dumper.DumpModemLogs(); + return 0; } diff --git a/modem/include/android_property_manager.h b/modem/include/android_property_manager.h new file mode 100644 index 0000000..7135d66 --- /dev/null +++ b/modem/include/android_property_manager.h @@ -0,0 +1,21 @@ +#pragma once + +#include + +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 diff --git a/modem/include/dumper.h b/modem/include/dumper.h new file mode 100644 index 0000000..348e666 --- /dev/null +++ b/modem/include/dumper.h @@ -0,0 +1,71 @@ +#pragma once + +#include +#include + +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 diff --git a/modem/include/modem_log_constants.h b/modem/include/modem_log_constants.h new file mode 100644 index 0000000..29a0fa8 --- /dev/null +++ b/modem/include/modem_log_constants.h @@ -0,0 +1,56 @@ +#pragma once +#include + +#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 diff --git a/modem/include/modem_log_dumper.h b/modem/include/modem_log_dumper.h new file mode 100644 index 0000000..96911b0 --- /dev/null +++ b/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 diff --git a/modem/modem_log_dumper.cpp b/modem/modem_log_dumper.cpp new file mode 100644 index 0000000..fad8d29 --- /dev/null +++ b/modem/modem_log_dumper.cpp @@ -0,0 +1,80 @@ +#include "modem_log_dumper.h" + +#include + +#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 diff --git a/modem/test/include/fake_android_property_manager.h b/modem/test/include/fake_android_property_manager.h new file mode 100644 index 0000000..79fd4f1 --- /dev/null +++ b/modem/test/include/fake_android_property_manager.h @@ -0,0 +1,77 @@ + + +#include +#include +#include + +#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 property_map_; + bool modem_logging_has_been_off_ = false; + bool modem_logging_has_restarted_ = false; +}; +} // namespace logging +} // namespace modem diff --git a/modem/test/modem_log_dumper_test.cpp b/modem/test/modem_log_dumper_test.cpp new file mode 100644 index 0000000..a052d43 --- /dev/null +++ b/modem/test/modem_log_dumper_test.cpp @@ -0,0 +1,106 @@ +#include "modem_log_dumper.h" + +#include + +#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