From 79283ef377e9253065d28998fb23a11a92a58537 Mon Sep 17 00:00:00 2001 From: Dennis Shen Date: Tue, 24 Oct 2023 17:30:12 +0000 Subject: [PATCH] Move staging value application logic to persistent_properties and add unit tests 1, Previous implementation has the staged prop application done in property_service, which caused a number of unnecessary changes which including exposing apis like AddPersistentProperty. In addition, it made the property_service logic complicated. A better design is to have the staged value application done while reading the persistent properties from file. This way, no change to property service. In addition, unit test is much cleaner and efficient. 2, add a unit test to lock down the behavior. Specifically, it locks down that when a prop is staged, it should be applied the next time when the persistent prop is loaded. In addition, it should lock down that other persistent props are not overwritten. Bug: b/307752841, b/300111812, b/306062513 Change-Id: I43c603efbb803195065dda3f0bc2145716302bbc --- init/persistent_properties.cpp | 65 +++++++++++++++++++++++++---- init/persistent_properties.h | 2 - init/persistent_properties_test.cpp | 32 ++++++++++++++ init/property_service.cpp | 23 +--------- 4 files changed, 90 insertions(+), 32 deletions(-) diff --git a/init/persistent_properties.cpp b/init/persistent_properties.cpp index e89244f13..6f8a4de93 100644 --- a/init/persistent_properties.cpp +++ b/init/persistent_properties.cpp @@ -46,6 +46,13 @@ namespace { constexpr const char kLegacyPersistentPropertyDir[] = "/data/property"; +void AddPersistentProperty(const std::string& name, const std::string& value, + PersistentProperties* persistent_properties) { + auto persistent_property_record = persistent_properties->add_properties(); + persistent_property_record->set_name(name); + persistent_property_record->set_value(value); +} + Result LoadLegacyPersistentProperties() { std::unique_ptr dir(opendir(kLegacyPersistentPropertyDir), closedir); if (!dir) { @@ -146,13 +153,6 @@ Result ParsePersistentPropertyFile(const std::string& file } // namespace -void AddPersistentProperty(const std::string& name, const std::string& value, - PersistentProperties* persistent_properties) { - auto persistent_property_record = persistent_properties->add_properties(); - persistent_property_record->set_name(name); - persistent_property_record->set_value(value); -} - Result LoadPersistentPropertyFile() { auto file_contents = ReadPersistentPropertyFile(); if (!file_contents.ok()) return file_contents.error(); @@ -266,8 +266,57 @@ PersistentProperties LoadPersistentProperties() { } } - return *persistent_properties; + // loop over to find all staged props + auto const staged_prefix = std::string_view("next_boot."); + auto staged_props = std::unordered_map(); + for (const auto& property_record : persistent_properties->properties()) { + auto const& prop_name = property_record.name(); + auto const& prop_value = property_record.value(); + if (StartsWith(prop_name, staged_prefix)) { + auto actual_prop_name = prop_name.substr(staged_prefix.size()); + staged_props[actual_prop_name] = prop_value; + } + } + + if (staged_props.empty()) { + return *persistent_properties; + } + + // if has staging, apply staging and perserve the original prop order + PersistentProperties updated_persistent_properties; + for (const auto& property_record : persistent_properties->properties()) { + auto const& prop_name = property_record.name(); + auto const& prop_value = property_record.value(); + + // don't include staged props anymore + if (StartsWith(prop_name, staged_prefix)) { + continue; + } + + auto iter = staged_props.find(prop_name); + if (iter != staged_props.end()) { + AddPersistentProperty(prop_name, iter->second, &updated_persistent_properties); + staged_props.erase(iter); + } else { + AddPersistentProperty(prop_name, prop_value, &updated_persistent_properties); + } + } + + // add any additional staged props + for (auto const& [prop_name, prop_value] : staged_props) { + AddPersistentProperty(prop_name, prop_value, &updated_persistent_properties); + } + + // write current updated persist prop file + auto result = WritePersistentPropertyFile(updated_persistent_properties); + if (!result.ok()) { + LOG(ERROR) << "Could not store persistent property: " << result.error(); + } + + return updated_persistent_properties; } + + } // namespace init } // namespace android diff --git a/init/persistent_properties.h b/init/persistent_properties.h index 7e9d4387a..11083da9e 100644 --- a/init/persistent_properties.h +++ b/init/persistent_properties.h @@ -25,8 +25,6 @@ namespace android { namespace init { -void AddPersistentProperty(const std::string& name, const std::string& value, - PersistentProperties* persistent_properties); PersistentProperties LoadPersistentProperties(); void WritePersistentProperty(const std::string& name, const std::string& value); PersistentProperties LoadPersistentPropertiesFromMemory(); diff --git a/init/persistent_properties_test.cpp b/init/persistent_properties_test.cpp index e5d26dbef..576305081 100644 --- a/init/persistent_properties_test.cpp +++ b/init/persistent_properties_test.cpp @@ -178,5 +178,37 @@ TEST(persistent_properties, RejectNonPersistProperty) { EXPECT_FALSE(it == read_back_properties.properties().end()); } +TEST(persistent_properties, StagedPersistProperty) { + TemporaryFile tf; + ASSERT_TRUE(tf.fd != -1); + persistent_property_filename = tf.path; + + std::vector> persistent_properties = { + {"persist.sys.locale", "en-US"}, + {"next_boot.persist.test.numbers", "54321"}, + {"persist.sys.timezone", "America/Los_Angeles"}, + {"persist.test.numbers", "12345"}, + {"next_boot.persist.test.extra", "abc"}, + }; + + ASSERT_RESULT_OK( + WritePersistentPropertyFile(VectorToPersistentProperties(persistent_properties))); + + std::vector> expected_persistent_properties = { + {"persist.sys.locale", "en-US"}, + {"persist.sys.timezone", "America/Los_Angeles"}, + {"persist.test.numbers", "54321"}, + {"persist.test.extra", "abc"}, + }; + + // lock down that staged props are applied + auto first_read_back_properties = LoadPersistentProperties(); + CheckPropertiesEqual(expected_persistent_properties, first_read_back_properties); + + // lock down that other props are not overwritten + auto second_read_back_properties = LoadPersistentProperties(); + CheckPropertiesEqual(expected_persistent_properties, second_read_back_properties); +} + } // namespace init } // namespace android diff --git a/init/property_service.cpp b/init/property_service.cpp index b08a904a7..87fc4c706 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -1396,33 +1396,12 @@ static void HandleInitSocket() { switch (init_message.msg_case()) { case InitMessage::kLoadPersistentProperties: { load_override_properties(); - // Read persistent properties after all default values have been loaded. - // Apply staged and persistent properties - bool has_staged_prop = false; - auto const staged_prefix = std::string_view("next_boot."); auto persistent_properties = LoadPersistentProperties(); for (const auto& property_record : persistent_properties.properties()) { auto const& prop_name = property_record.name(); auto const& prop_value = property_record.value(); - - if (StartsWith(prop_name, staged_prefix)) { - has_staged_prop = true; - auto actual_prop_name = prop_name.substr(staged_prefix.size()); - InitPropertySet(actual_prop_name, prop_value); - } else { - InitPropertySet(prop_name, prop_value); - } - } - - // Update persist prop file if there are staged props - if (has_staged_prop) { - PersistentProperties props = LoadPersistentPropertiesFromMemory(); - // write current updated persist prop file - auto result = WritePersistentPropertyFile(props); - if (!result.ok()) { - LOG(ERROR) << "Could not store persistent property: " << result.error(); - } + InitPropertySet(prop_name, prop_value); } // Apply debug ramdisk special settings after persistent properties are loaded.