From fe5065705ca7e96b74e92911653a4acf3a9df5bd Mon Sep 17 00:00:00 2001 From: Dennis Shen Date: Tue, 23 Apr 2024 21:31:37 +0000 Subject: [PATCH] aconfig: update aconfig storage write api and test update Simplify storage write api so that we don't need the storage records pb file. Bug: b/312444587 Test: atest -c Change-Id: I7e336b1d7766983364715dae15786b91b0c0743f --- tools/aconfig/aconfig/src/commands.rs | 6 +- tools/aconfig/aconfig/src/storage/mod.rs | 4 +- .../aconfig/tests/storage_test_2.aconfig | 2 +- .../aconfig/tests/storage_test_2.values | 4 +- .../aconfig/tests/storage_test_4.aconfig | 2 +- .../aconfig/tests/storage_test_4.values | 4 +- tools/aconfig/aconfig_storage_file/src/lib.rs | 8 +- .../aconfig_storage_file/src/test_utils.rs | 8 +- .../aconfig_storage_read_api.cpp | 34 ++--- .../aconfig_storage_read_api.hpp | 5 + .../src/flag_info_query.rs | 2 +- .../src/flag_table_query.rs | 4 +- .../aconfig_storage_read_api/src/lib.rs | 4 +- .../aconfig_storage_read_api/tests/flag.map | Bin 321 -> 321 bytes .../tests/storage_read_api_test.cpp | 4 +- .../tests/storage_read_api_test.rs | 4 +- .../aconfig_storage_write_api.cpp | 80 ----------- .../aconfig_storage_write_api.hpp | 15 -- .../aconfig_storage_write_api/src/lib.rs | 85 +----------- .../src/mapped_file.rs | 131 ++---------------- .../aconfig_storage_write_api/tests/flag.info | Bin 35 -> 35 bytes .../tests/storage_write_api_test.cpp | 70 ++-------- .../tests/storage_write_api_test.rs | 57 +------- 23 files changed, 79 insertions(+), 454 deletions(-) diff --git a/tools/aconfig/aconfig/src/commands.rs b/tools/aconfig/aconfig/src/commands.rs index 9cb40bf917..ad96bb88a5 100644 --- a/tools/aconfig/aconfig/src/commands.rs +++ b/tools/aconfig/aconfig/src/commands.rs @@ -239,10 +239,8 @@ pub fn create_storage( container: &str, file: &StorageFileType, ) -> Result> { - let parsed_flags_vec: Vec = caches - .into_iter() - .map(|mut input| input.try_parse_flags()) - .collect::>>()?; + let parsed_flags_vec: Vec = + caches.into_iter().map(|mut input| input.try_parse_flags()).collect::>>()?; generate_storage_file(container, parsed_flags_vec.iter(), file) } diff --git a/tools/aconfig/aconfig/src/storage/mod.rs b/tools/aconfig/aconfig/src/storage/mod.rs index 855ed0235a..73339f24b3 100644 --- a/tools/aconfig/aconfig/src/storage/mod.rs +++ b/tools/aconfig/aconfig/src/storage/mod.rs @@ -189,14 +189,14 @@ mod tests { assert_eq!(packages[1].package_id, 1); assert_eq!(packages[1].flag_names.len(), 3); assert!(packages[1].flag_names.contains("enabled_ro")); - assert!(packages[1].flag_names.contains("disabled_ro")); + assert!(packages[1].flag_names.contains("disabled_rw")); assert!(packages[1].flag_names.contains("enabled_fixed_ro")); assert_eq!(packages[1].boolean_start_index, 3); assert_eq!(packages[2].package_name, "com.android.aconfig.storage.test_4"); assert_eq!(packages[2].package_id, 2); assert_eq!(packages[2].flag_names.len(), 2); - assert!(packages[2].flag_names.contains("enabled_ro")); + assert!(packages[2].flag_names.contains("enabled_rw")); assert!(packages[2].flag_names.contains("enabled_fixed_ro")); assert_eq!(packages[2].boolean_start_index, 6); } diff --git a/tools/aconfig/aconfig/tests/storage_test_2.aconfig b/tools/aconfig/aconfig/tests/storage_test_2.aconfig index bb14fd1324..db77f7a35c 100644 --- a/tools/aconfig/aconfig/tests/storage_test_2.aconfig +++ b/tools/aconfig/aconfig/tests/storage_test_2.aconfig @@ -9,7 +9,7 @@ flag { } flag { - name: "disabled_ro" + name: "disabled_rw" namespace: "aconfig_test" description: "This flag is DISABLED + READ_ONLY" bug: "123" diff --git a/tools/aconfig/aconfig/tests/storage_test_2.values b/tools/aconfig/aconfig/tests/storage_test_2.values index a7bb0b1a3a..b6507210da 100644 --- a/tools/aconfig/aconfig/tests/storage_test_2.values +++ b/tools/aconfig/aconfig/tests/storage_test_2.values @@ -6,9 +6,9 @@ flag_value { } flag_value { package: "com.android.aconfig.storage.test_2" - name: "disabled_ro" + name: "disabled_rw" state: DISABLED - permission: READ_ONLY + permission: READ_WRITE } flag_value { package: "com.android.aconfig.storage.test_2" diff --git a/tools/aconfig/aconfig/tests/storage_test_4.aconfig b/tools/aconfig/aconfig/tests/storage_test_4.aconfig index 333fe09e8e..5802a73b6d 100644 --- a/tools/aconfig/aconfig/tests/storage_test_4.aconfig +++ b/tools/aconfig/aconfig/tests/storage_test_4.aconfig @@ -2,7 +2,7 @@ package: "com.android.aconfig.storage.test_4" container: "system" flag { - name: "enabled_ro" + name: "enabled_rw" namespace: "aconfig_test" description: "This flag is ENABLED + READ_ONLY" bug: "abc" diff --git a/tools/aconfig/aconfig/tests/storage_test_4.values b/tools/aconfig/aconfig/tests/storage_test_4.values index fa21317c43..784b744f62 100644 --- a/tools/aconfig/aconfig/tests/storage_test_4.values +++ b/tools/aconfig/aconfig/tests/storage_test_4.values @@ -1,8 +1,8 @@ flag_value { package: "com.android.aconfig.storage.test_4" - name: "enabled_ro" + name: "enabled_rw" state: ENABLED - permission: READ_ONLY + permission: READ_WRITE } flag_value { package: "com.android.aconfig.storage.test_4" diff --git a/tools/aconfig/aconfig_storage_file/src/lib.rs b/tools/aconfig/aconfig_storage_file/src/lib.rs index 070a3cfc17..2acfc7dbc7 100644 --- a/tools/aconfig/aconfig_storage_file/src/lib.rs +++ b/tools/aconfig/aconfig_storage_file/src/lib.rs @@ -357,8 +357,8 @@ mod tests { ), ( String::from("com.android.aconfig.storage.test_2"), - String::from("disabled_ro"), - StoredFlagType::ReadOnlyBoolean, + String::from("disabled_rw"), + StoredFlagType::ReadWriteBoolean, false, ), ( @@ -381,8 +381,8 @@ mod tests { ), ( String::from("com.android.aconfig.storage.test_4"), - String::from("enabled_ro"), - StoredFlagType::ReadOnlyBoolean, + String::from("enabled_rw"), + StoredFlagType::ReadWriteBoolean, true, ), ]; diff --git a/tools/aconfig/aconfig_storage_file/src/test_utils.rs b/tools/aconfig/aconfig_storage_file/src/test_utils.rs index 608563c498..106666c47f 100644 --- a/tools/aconfig/aconfig_storage_file/src/test_utils.rs +++ b/tools/aconfig/aconfig_storage_file/src/test_utils.rs @@ -92,8 +92,8 @@ pub fn create_test_flag_table() -> FlagTable { None, None, None, - Some(178), None, + Some(177), Some(204), None, Some(262), @@ -108,8 +108,8 @@ pub fn create_test_flag_table() -> FlagTable { let nodes = vec![ FlagTableNode::new_expected(0, "enabled_ro", 1, 1, None), FlagTableNode::new_expected(0, "enabled_rw", 0, 2, Some(151)), - FlagTableNode::new_expected(1, "disabled_ro", 1, 0, None), - FlagTableNode::new_expected(2, "enabled_ro", 1, 1, None), + FlagTableNode::new_expected(2, "enabled_rw", 0, 1, None), + FlagTableNode::new_expected(1, "disabled_rw", 0, 0, None), FlagTableNode::new_expected(1, "enabled_fixed_ro", 2, 1, Some(236)), FlagTableNode::new_expected(1, "enabled_ro", 1, 2, None), FlagTableNode::new_expected(2, "enabled_fixed_ro", 2, 0, None), @@ -140,7 +140,7 @@ pub fn create_test_flag_info_list() -> FlagInfoList { num_flags: 8, boolean_flag_offset: 27, }; - let is_flag_rw = [true, false, true, false, false, false, false, false]; + let is_flag_rw = [true, false, true, true, false, false, false, true]; let nodes = is_flag_rw.iter().map(|&rw| FlagInfoNode::create(rw)).collect(); FlagInfoList { header, nodes } } diff --git a/tools/aconfig/aconfig_storage_read_api/aconfig_storage_read_api.cpp b/tools/aconfig/aconfig_storage_read_api/aconfig_storage_read_api.cpp index ff2f38e765..8fe42ceddc 100644 --- a/tools/aconfig/aconfig_storage_read_api/aconfig_storage_read_api.cpp +++ b/tools/aconfig/aconfig_storage_read_api/aconfig_storage_read_api.cpp @@ -65,8 +65,24 @@ static Result find_storage_file( return Error() << "Unable to find storage files for container " << container;; } +namespace private_internal_api { + +/// Get mapped file implementation. +Result get_mapped_file_impl( + std::string const& pb_file, + std::string const& container, + StorageFileType file_type) { + auto file_result = find_storage_file(pb_file, container, file_type); + if (!file_result.ok()) { + return Error() << file_result.error(); + } + return map_storage_file(*file_result); +} + +} // namespace private internal api + /// Map a storage file -static Result map_storage_file(std::string const& file) { +Result map_storage_file(std::string const& file) { int fd = open(file.c_str(), O_CLOEXEC | O_NOFOLLOW | O_RDONLY); if (fd == -1) { return ErrnoError() << "failed to open " << file; @@ -90,22 +106,6 @@ static Result map_storage_file(std::string const& file) { return mapped_file; } -namespace private_internal_api { - -/// Get mapped file implementation. -Result get_mapped_file_impl( - std::string const& pb_file, - std::string const& container, - StorageFileType file_type) { - auto file_result = find_storage_file(pb_file, container, file_type); - if (!file_result.ok()) { - return Error() << file_result.error(); - } - return map_storage_file(*file_result); -} - -} // namespace private internal api - /// Map from StoredFlagType to FlagValueType android::base::Result map_to_flag_value_type( StoredFlagType stored_type) { diff --git a/tools/aconfig/aconfig_storage_read_api/include/aconfig_storage/aconfig_storage_read_api.hpp b/tools/aconfig/aconfig_storage_read_api/include/aconfig_storage/aconfig_storage_read_api.hpp index 0a4d5b4501..2bd84fcf20 100644 --- a/tools/aconfig/aconfig_storage_read_api/include/aconfig_storage/aconfig_storage_read_api.hpp +++ b/tools/aconfig/aconfig_storage_read_api/include/aconfig_storage/aconfig_storage_read_api.hpp @@ -67,6 +67,11 @@ android::base::Result get_mapped_file_impl( } // namespace private_internal_api +/// Map a storage file +android::base::Result map_storage_file( + std::string const& file); + + /// Map from StoredFlagType to FlagValueType /// \input stored_type: stored flag type in the storage file /// \returns the flag value type enum diff --git a/tools/aconfig/aconfig_storage_read_api/src/flag_info_query.rs b/tools/aconfig/aconfig_storage_read_api/src/flag_info_query.rs index 0087febce1..6d03377683 100644 --- a/tools/aconfig/aconfig_storage_read_api/src/flag_info_query.rs +++ b/tools/aconfig/aconfig_storage_read_api/src/flag_info_query.rs @@ -70,7 +70,7 @@ mod tests { // this test point locks down query if flag is readwrite fn test_is_flag_readwrite() { let flag_info_list = create_test_flag_info_list().into_bytes(); - let baseline: Vec = vec![true, false, true, false, false, false, false, false]; + let baseline: Vec = vec![true, false, true, true, false, false, false, true]; for offset in 0..8 { let attribute = find_flag_attribute(&flag_info_list[..], FlagValueType::Boolean, offset).unwrap(); diff --git a/tools/aconfig/aconfig_storage_read_api/src/flag_table_query.rs b/tools/aconfig/aconfig_storage_read_api/src/flag_table_query.rs index 55fdcb7606..a1a4793bc2 100644 --- a/tools/aconfig/aconfig_storage_read_api/src/flag_table_query.rs +++ b/tools/aconfig/aconfig_storage_read_api/src/flag_table_query.rs @@ -82,8 +82,8 @@ mod tests { let baseline = vec![ (0, "enabled_ro", StoredFlagType::ReadOnlyBoolean, 1u16), (0, "enabled_rw", StoredFlagType::ReadWriteBoolean, 2u16), - (1, "disabled_ro", StoredFlagType::ReadOnlyBoolean, 0u16), - (2, "enabled_ro", StoredFlagType::ReadOnlyBoolean, 1u16), + (2, "enabled_rw", StoredFlagType::ReadWriteBoolean, 1u16), + (1, "disabled_rw", StoredFlagType::ReadWriteBoolean, 0u16), (1, "enabled_fixed_ro", StoredFlagType::FixedReadOnlyBoolean, 1u16), (1, "enabled_ro", StoredFlagType::ReadOnlyBoolean, 2u16), (2, "enabled_fixed_ro", StoredFlagType::FixedReadOnlyBoolean, 0u16), diff --git a/tools/aconfig/aconfig_storage_read_api/src/lib.rs b/tools/aconfig/aconfig_storage_read_api/src/lib.rs index fa8dad6de6..e65dcfbc69 100644 --- a/tools/aconfig/aconfig_storage_read_api/src/lib.rs +++ b/tools/aconfig/aconfig_storage_read_api/src/lib.rs @@ -468,8 +468,8 @@ files {{ let baseline = vec![ (0, "enabled_ro", StoredFlagType::ReadOnlyBoolean, 1u16), (0, "enabled_rw", StoredFlagType::ReadWriteBoolean, 2u16), - (1, "disabled_ro", StoredFlagType::ReadOnlyBoolean, 0u16), - (2, "enabled_ro", StoredFlagType::ReadOnlyBoolean, 1u16), + (2, "enabled_rw", StoredFlagType::ReadWriteBoolean, 1u16), + (1, "disabled_rw", StoredFlagType::ReadWriteBoolean, 0u16), (1, "enabled_fixed_ro", StoredFlagType::FixedReadOnlyBoolean, 1u16), (1, "enabled_ro", StoredFlagType::ReadOnlyBoolean, 2u16), (2, "enabled_fixed_ro", StoredFlagType::FixedReadOnlyBoolean, 0u16), diff --git a/tools/aconfig/aconfig_storage_read_api/tests/flag.map b/tools/aconfig/aconfig_storage_read_api/tests/flag.map index d26e00f5dcddedc0409fac9d8787268fd3346641..e868f53d7e599f130604faac1fd20149e73cb821 100644 GIT binary patch delta 67 zcmX@ebdYI+F)ITQY@BEjZ@>g(Z~<{@USd*CYD#=jIRgVDNES?T1L>5^VuTdK#N$-} D-Zu`` delta 67 zcmX@ebdYI+G3zDtoxq&z(vp6v+Cp9I$C?6yQ0!%>61;nX&2pPtS$EyJ5 C^$y7Z diff --git a/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.cpp b/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.cpp index 5b4d32cd14..b499c1c279 100644 --- a/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.cpp +++ b/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.cpp @@ -167,8 +167,8 @@ TEST_F(AconfigStorageTest, test_flag_context_query) { auto baseline = std::vector>{ {0, "enabled_ro", api::StoredFlagType::ReadOnlyBoolean, 1}, {0, "enabled_rw", api::StoredFlagType::ReadWriteBoolean, 2}, - {1, "disabled_ro", api::StoredFlagType::ReadOnlyBoolean, 0}, - {2, "enabled_ro", api::StoredFlagType::ReadOnlyBoolean, 1}, + {2, "enabled_rw", api::StoredFlagType::ReadWriteBoolean, 1}, + {1, "disabled_rw", api::StoredFlagType::ReadWriteBoolean, 0}, {1, "enabled_fixed_ro", api::StoredFlagType::FixedReadOnlyBoolean, 1}, {1, "enabled_ro", api::StoredFlagType::ReadOnlyBoolean, 2}, {2, "enabled_fixed_ro", api::StoredFlagType::FixedReadOnlyBoolean, 0}, diff --git a/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.rs b/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.rs index 1a8af7588e..ce9c0186dd 100644 --- a/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.rs +++ b/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.rs @@ -118,8 +118,8 @@ files {{ let baseline = vec![ (0, "enabled_ro", StoredFlagType::ReadOnlyBoolean, 1u16), (0, "enabled_rw", StoredFlagType::ReadWriteBoolean, 2u16), - (1, "disabled_ro", StoredFlagType::ReadOnlyBoolean, 0u16), - (2, "enabled_ro", StoredFlagType::ReadOnlyBoolean, 1u16), + (2, "enabled_rw", StoredFlagType::ReadWriteBoolean, 1u16), + (1, "disabled_rw", StoredFlagType::ReadWriteBoolean, 0u16), (1, "enabled_fixed_ro", StoredFlagType::FixedReadOnlyBoolean, 1u16), (1, "enabled_ro", StoredFlagType::ReadOnlyBoolean, 2u16), (2, "enabled_fixed_ro", StoredFlagType::FixedReadOnlyBoolean, 0u16), diff --git a/tools/aconfig/aconfig_storage_write_api/aconfig_storage_write_api.cpp b/tools/aconfig/aconfig_storage_write_api/aconfig_storage_write_api.cpp index f031d014c5..cd57f4fb29 100644 --- a/tools/aconfig/aconfig_storage_write_api/aconfig_storage_write_api.cpp +++ b/tools/aconfig/aconfig_storage_write_api/aconfig_storage_write_api.cpp @@ -17,78 +17,6 @@ using namespace android::base; namespace aconfig_storage { -/// Storage location pb file -static constexpr char kPersistStorageRecordsPb[] = - "/metadata/aconfig/persistent_storage_file_records.pb"; - -/// Read aconfig storage records pb file -static Result read_storage_records_pb(std::string const& pb_file) { - auto records = storage_records_pb(); - auto content = std::string(); - if (!ReadFileToString(pb_file, &content)) { - return ErrnoError() << "ReadFileToString failed"; - } - - if (!records.ParseFromString(content)) { - return ErrnoError() << "Unable to parse persistent storage records protobuf"; - } - return records; -} - -/// Get storage file path -static Result find_storage_file( - std::string const& pb_file, - std::string const& container, - StorageFileType file_type) { - auto records_pb = read_storage_records_pb(pb_file); - if (!records_pb.ok()) { - return Error() << "Unable to read storage records from " << pb_file - << " : " << records_pb.error(); - } - - for (auto& entry : records_pb->files()) { - if (entry.container() == container) { - switch(file_type) { - case StorageFileType::package_map: - return entry.package_map(); - case StorageFileType::flag_map: - return entry.flag_map(); - case StorageFileType::flag_val: - return entry.flag_val(); - case StorageFileType::flag_info: - return entry.flag_info(); - default: - return Error() << "Invalid file type " << file_type; - } - } - } - - return Error() << "Unable to find storage files for container " << container; -} - - -namespace private_internal_api { - -/// Get mutable mapped file implementation. -Result get_mutable_mapped_file_impl( - std::string const& pb_file, - std::string const& container, - StorageFileType file_type) { - if (file_type != StorageFileType::flag_val && - file_type != StorageFileType::flag_info) { - return Error() << "Cannot create mutable mapped file for this file type"; - } - - auto file_result = find_storage_file(pb_file, container, file_type); - if (!file_result.ok()) { - return Error() << file_result.error(); - } - - return map_mutable_storage_file(*file_result); -} - -} // namespace private internal api - /// Map a storage file Result map_mutable_storage_file(std::string const& file) { struct stat file_stat; @@ -120,14 +48,6 @@ Result map_mutable_storage_file(std::string const& fil return mapped_file; } -/// Get mutable mapped file -Result get_mutable_mapped_file( - std::string const& container, - StorageFileType file_type) { - return private_internal_api::get_mutable_mapped_file_impl( - kPersistStorageRecordsPb, container, file_type); -} - /// Set boolean flag value Result set_boolean_flag_value( const MutableMappedStorageFile& file, diff --git a/tools/aconfig/aconfig_storage_write_api/include/aconfig_storage/aconfig_storage_write_api.hpp b/tools/aconfig/aconfig_storage_write_api/include/aconfig_storage/aconfig_storage_write_api.hpp index 12b0ecceb0..7148396b82 100644 --- a/tools/aconfig/aconfig_storage_write_api/include/aconfig_storage/aconfig_storage_write_api.hpp +++ b/tools/aconfig/aconfig_storage_write_api/include/aconfig_storage/aconfig_storage_write_api.hpp @@ -16,25 +16,10 @@ struct MutableMappedStorageFile{ size_t file_size; }; -/// DO NOT USE APIS IN THE FOLLOWING NAMESPACE DIRECTLY -namespace private_internal_api { - -Result get_mutable_mapped_file_impl( - std::string const& pb_file, - std::string const& container, - StorageFileType file_type); - -} // namespace private_internal_api - /// Map a storage file Result map_mutable_storage_file( std::string const& file); -/// Get mapped writeable storage file -Result get_mutable_mapped_file( - std::string const& container, - StorageFileType file_type); - /// Set boolean flag value Result set_boolean_flag_value( const MutableMappedStorageFile& file, diff --git a/tools/aconfig/aconfig_storage_write_api/src/lib.rs b/tools/aconfig/aconfig_storage_write_api/src/lib.rs index bcddce1159..aec28def68 100644 --- a/tools/aconfig/aconfig_storage_write_api/src/lib.rs +++ b/tools/aconfig/aconfig_storage_write_api/src/lib.rs @@ -34,15 +34,9 @@ use memmap2::MmapMut; use std::fs::File; use std::io::{Read, Write}; -/// Storage file location pb file -pub const STORAGE_LOCATION_FILE: &str = "/metadata/aconfig/persistent_storage_file_records.pb"; - /// Get read write mapped storage files. /// -/// \input container: the flag package container -/// \input file_type: storage file type enum -/// \return a result of read write mapped file -/// +/// \input file_path: path to the storage file /// /// # Safety /// @@ -50,11 +44,8 @@ pub const STORAGE_LOCATION_FILE: &str = "/metadata/aconfig/persistent_storage_fi /// file not thru this memory mapped file or there are concurrent writes to this /// memory mapped file. Ensure all writes to the underlying file are thru this memory /// mapped file and there are no concurrent writes. -pub unsafe fn get_mapped_storage_file( - container: &str, - file_type: StorageFileType, -) -> Result { - unsafe { crate::mapped_file::get_mapped_file(STORAGE_LOCATION_FILE, container, file_type) } +pub unsafe fn map_mutable_storage_file(file_path: &str) -> Result { + crate::mapped_file::map_file(file_path) } /// Set boolean flag value thru mapped file and flush the change to file @@ -344,7 +335,6 @@ pub(crate) fn create_flag_info_cxx( mod tests { use super::*; use crate::test_utils::copy_to_temp_file; - use aconfig_storage_file::protos::storage_record_pb::write_proto_to_temp_file; use aconfig_storage_file::test_utils::{ create_test_flag_info_list, create_test_flag_table, create_test_package_table, write_bytes_to_temp_file, @@ -367,33 +357,12 @@ mod tests { fn test_set_boolean_flag_value() { let flag_value_file = copy_to_temp_file("./tests/flag.val", false).unwrap(); let flag_value_path = flag_value_file.path().display().to_string(); - let text_proto = format!( - r#" -files {{ - version: 0 - container: "system" - package_map: "some_package.map" - flag_map: "some_flag.map" - flag_val: "{}" - flag_info: "some_flag.info" - timestamp: 12345 -}} -"#, - flag_value_path - ); - let record_pb_file = write_proto_to_temp_file(&text_proto).unwrap(); - let record_pb_path = record_pb_file.path().display().to_string(); // SAFETY: // The safety here is guaranteed as only this single threaded test process will // write to this file unsafe { - let mut file = crate::mapped_file::get_mapped_file( - &record_pb_path, - "system", - StorageFileType::FlagVal, - ) - .unwrap(); + let mut file = map_mutable_storage_file(&flag_value_path).unwrap(); for i in 0..8 { set_boolean_flag_value(&mut file, i, true).unwrap(); let value = get_boolean_flag_value_at_offset(&flag_value_path, i); @@ -417,33 +386,12 @@ files {{ fn test_set_flag_has_server_override() { let flag_info_file = copy_to_temp_file("./tests/flag.info", false).unwrap(); let flag_info_path = flag_info_file.path().display().to_string(); - let text_proto = format!( - r#" - files {{ - version: 0 - container: "system" - package_map: "some_package.map" - flag_map: "some_flag.map" - flag_val: "some_flag.val" - flag_info: "{}" - timestamp: 12345 - }} - "#, - flag_info_path - ); - let record_pb_file = write_proto_to_temp_file(&text_proto).unwrap(); - let record_pb_path = record_pb_file.path().display().to_string(); // SAFETY: // The safety here is guaranteed as only this single threaded test process will // write to this file unsafe { - let mut file = crate::mapped_file::get_mapped_file( - &record_pb_path, - "system", - StorageFileType::FlagInfo, - ) - .unwrap(); + let mut file = map_mutable_storage_file(&flag_info_path).unwrap(); for i in 0..8 { set_flag_has_server_override(&mut file, FlagValueType::Boolean, i, true).unwrap(); let attribute = @@ -461,33 +409,12 @@ files {{ fn test_set_flag_has_local_override() { let flag_info_file = copy_to_temp_file("./tests/flag.info", false).unwrap(); let flag_info_path = flag_info_file.path().display().to_string(); - let text_proto = format!( - r#" - files {{ - version: 0 - container: "system" - package_map: "some_package.map" - flag_map: "some_flag.map" - flag_val: "some_flag.val" - flag_info: "{}" - timestamp: 12345 - }} - "#, - flag_info_path - ); - let record_pb_file = write_proto_to_temp_file(&text_proto).unwrap(); - let record_pb_path = record_pb_file.path().display().to_string(); // SAFETY: // The safety here is guaranteed as only this single threaded test process will // write to this file unsafe { - let mut file = crate::mapped_file::get_mapped_file( - &record_pb_path, - "system", - StorageFileType::FlagInfo, - ) - .unwrap(); + let mut file = map_mutable_storage_file(&flag_info_path).unwrap(); for i in 0..8 { set_flag_has_local_override(&mut file, FlagValueType::Boolean, i, true).unwrap(); let attribute = diff --git a/tools/aconfig/aconfig_storage_write_api/src/mapped_file.rs b/tools/aconfig/aconfig_storage_write_api/src/mapped_file.rs index ea9ac199af..401d6b798a 100644 --- a/tools/aconfig/aconfig_storage_write_api/src/mapped_file.rs +++ b/tools/aconfig/aconfig_storage_write_api/src/mapped_file.rs @@ -17,11 +17,8 @@ use anyhow::anyhow; use memmap2::MmapMut; use std::fs::{self, OpenOptions}; -use std::io::Read; use aconfig_storage_file::AconfigStorageError::{self, FileReadFail, MapFileFail}; -use aconfig_storage_file::StorageFileType; -use aconfig_storage_read_api::mapped_file::find_container_storage_location; /// Get the mutable memory mapping of a storage file /// @@ -31,7 +28,7 @@ use aconfig_storage_read_api::mapped_file::find_container_storage_location; /// file not thru this memory mapped file or there are concurrent writes to this /// memory mapped file. Ensure all writes to the underlying file are thru this memory /// mapped file and there are no concurrent writes. -unsafe fn map_file(file_path: &str) -> Result { +pub(crate) unsafe fn map_file(file_path: &str) -> Result { // make sure file has read write permission let perms = fs::metadata(file_path).unwrap().permissions(); if perms.readonly() { @@ -51,57 +48,18 @@ unsafe fn map_file(file_path: &str) -> Result { } } -/// Get a mapped storage file given the container and file type -/// -/// # Safety -/// -/// The memory mapped file may have undefined behavior if there are writes to this -/// file not thru this memory mapped file or there are concurrent writes to this -/// memory mapped file. Ensure all writes to the underlying file are thru this memory -/// mapped file and there are no concurrent writes. -pub unsafe fn get_mapped_file( - location_pb_file: &str, - container: &str, - file_type: StorageFileType, -) -> Result { - let files_location = find_container_storage_location(location_pb_file, container)?; - match file_type { - StorageFileType::FlagVal => unsafe { map_file(files_location.flag_val()) }, - StorageFileType::FlagInfo => unsafe { map_file(files_location.flag_info()) }, - _ => Err(MapFileFail(anyhow!( - "Cannot map file type {:?} as writeable memory mapped files.", - file_type - ))), - } -} - #[cfg(test)] mod tests { use super::*; use crate::test_utils::copy_to_temp_file; - use aconfig_storage_file::protos::storage_record_pb::write_proto_to_temp_file; + use std::io::Read; #[test] fn test_mapped_file_contents() { let mut rw_val_file = copy_to_temp_file("./tests/flag.val", false).unwrap(); let mut rw_info_file = copy_to_temp_file("./tests/flag.info", false).unwrap(); - let text_proto = format!( - r#" -files {{ - version: 0 - container: "system" - package_map: "some_package.map" - flag_map: "some_flag.map" - flag_val: "{}" - flag_info: "{}" - timestamp: 12345 -}} -"#, - rw_val_file.path().display().to_string(), - rw_info_file.path().display().to_string() - ); - let storage_record_file = write_proto_to_temp_file(&text_proto).unwrap(); - let storage_record_file_path = storage_record_file.path().display().to_string(); + let flag_val = rw_val_file.path().display().to_string(); + let flag_info = rw_info_file.path().display().to_string(); let mut content = Vec::new(); rw_val_file.read_to_end(&mut content).unwrap(); @@ -109,9 +67,7 @@ files {{ // SAFETY: // The safety here is guaranteed here as no writes happens to this temp file unsafe { - let mmaped_file = - get_mapped_file(&storage_record_file_path, "system", StorageFileType::FlagVal) - .unwrap(); + let mmaped_file = map_file(&flag_val).unwrap(); assert_eq!(mmaped_file[..], content[..]); } @@ -121,90 +77,23 @@ files {{ // SAFETY: // The safety here is guaranteed here as no writes happens to this temp file unsafe { - let mmaped_file = - get_mapped_file(&storage_record_file_path, "system", StorageFileType::FlagInfo) - .unwrap(); + let mmaped_file = map_file(&flag_info).unwrap(); assert_eq!(mmaped_file[..], content[..]); } } #[test] fn test_mapped_read_only_file() { - let ro_file = copy_to_temp_file("./tests/flag.val", true).unwrap(); - let text_proto = format!( - r#" -files {{ - version: 0 - container: "system" - package_map: "some_package.map" - flag_map: "some_flag.map" - flag_val: "{}" - flag_info: "some_flag.info" - timestamp: 12345 -}} -"#, - ro_file.path().display().to_string() - ); - let storage_record_file = write_proto_to_temp_file(&text_proto).unwrap(); - let storage_record_file_path = storage_record_file.path().display().to_string(); + let ro_val_file = copy_to_temp_file("./tests/flag.val", true).unwrap(); + let flag_val = ro_val_file.path().display().to_string(); // SAFETY: // The safety here is guaranteed here as no writes happens to this temp file unsafe { - let error = - get_mapped_file(&storage_record_file_path, "system", StorageFileType::FlagVal) - .unwrap_err(); + let error = map_file(&flag_val).unwrap_err(); assert_eq!( format!("{:?}", error), - format!( - "MapFileFail(fail to map non read write storage file {})", - ro_file.path().display().to_string() - ) - ); - } - } - - #[test] - fn test_mapped_not_supported_file() { - let text_proto = format!( - r#" -files {{ - version: 0 - container: "system" - package_map: "some_package.map" - flag_map: "some_flag.map" - flag_val: "some_flag.val" - flag_info: "some_flag.info" - timestamp: 12345 -}} -"#, - ); - let storage_record_file = write_proto_to_temp_file(&text_proto).unwrap(); - let storage_record_file_path = storage_record_file.path().display().to_string(); - - // SAFETY: - // The safety here is guaranteed here as no writes happens to this temp file - unsafe { - let error = - get_mapped_file(&storage_record_file_path, "system", StorageFileType::PackageMap) - .unwrap_err(); - assert_eq!( - format!("{:?}", error), - format!( - "MapFileFail(Cannot map file type {:?} as writeable memory mapped files.)", - StorageFileType::PackageMap - ) - ); - - let error = - get_mapped_file(&storage_record_file_path, "system", StorageFileType::FlagMap) - .unwrap_err(); - assert_eq!( - format!("{:?}", error), - format!( - "MapFileFail(Cannot map file type {:?} as writeable memory mapped files.)", - StorageFileType::FlagMap - ) + format!("MapFileFail(fail to map non read write storage file {})", flag_val) ); } } diff --git a/tools/aconfig/aconfig_storage_write_api/tests/flag.info b/tools/aconfig/aconfig_storage_write_api/tests/flag.info index 820d8393633a9cc00b67edad843294b5c0826844..6223edf369cbd0635aa34de23a676b1cf8aa3667 100644 GIT binary patch delta 10 RcmY#Zo*>7{#K6G71ONyh0R8{~ delta 10 OcmY#Zo*>7{00ICA69D=E diff --git a/tools/aconfig/aconfig_storage_write_api/tests/storage_write_api_test.cpp b/tools/aconfig/aconfig_storage_write_api/tests/storage_write_api_test.cpp index ab5f0a7ffd..bd39e9e1de 100644 --- a/tools/aconfig/aconfig_storage_write_api/tests/storage_write_api_test.cpp +++ b/tools/aconfig/aconfig_storage_write_api/tests/storage_write_api_test.cpp @@ -50,88 +50,36 @@ class AconfigStorageTest : public ::testing::Test { return temp_file; } - Result write_storage_location_pb_file(std::string const& flag_val, - std::string const& flag_info) { - auto temp_file = std::tmpnam(nullptr); - auto proto = storage_files(); - auto* info = proto.add_files(); - info->set_version(0); - info->set_container("mockup"); - info->set_package_map("some_package.map"); - info->set_flag_map("some_flag.map"); - info->set_flag_val(flag_val); - info->set_flag_info(flag_info); - info->set_timestamp(12345); - - auto content = std::string(); - proto.SerializeToString(&content); - if (!WriteStringToFile(content, temp_file)) { - return Error() << "failed to write storage records pb file"; - } - return temp_file; - } - void SetUp() override { auto const test_dir = android::base::GetExecutableDirectory(); flag_val = *copy_to_rw_temp_file(test_dir + "/flag.val"); flag_info = *copy_to_rw_temp_file(test_dir + "/flag.info"); - storage_record_pb = *write_storage_location_pb_file(flag_val, flag_info); } void TearDown() override { std::remove(flag_val.c_str()); std::remove(flag_info.c_str()); - std::remove(storage_record_pb.c_str()); } std::string flag_val; std::string flag_info; - std::string storage_record_pb; }; -/// Negative test to lock down the error when mapping none exist storage files -TEST_F(AconfigStorageTest, test_none_exist_storage_file_mapping) { - auto mapped_file_result = private_api::get_mutable_mapped_file_impl( - storage_record_pb, "vendor", api::StorageFileType::flag_val); - ASSERT_FALSE(mapped_file_result.ok()); - ASSERT_EQ(mapped_file_result.error().message(), - "Unable to find storage files for container vendor"); -} - /// Negative test to lock down the error when mapping a non writeable storage file TEST_F(AconfigStorageTest, test_non_writable_storage_file_mapping) { ASSERT_TRUE(chmod(flag_val.c_str(), S_IRUSR | S_IRGRP | S_IROTH) != -1); - auto mapped_file_result = private_api::get_mutable_mapped_file_impl( - storage_record_pb, "mockup", api::StorageFileType::flag_val); + auto mapped_file_result = api::map_mutable_storage_file(flag_val); ASSERT_FALSE(mapped_file_result.ok()); auto it = mapped_file_result.error().message().find("cannot map nonwriteable file"); ASSERT_TRUE(it != std::string::npos) << mapped_file_result.error().message(); } -/// Negative test to lock down the error when mapping a file type that cannot be modified -TEST_F(AconfigStorageTest, test_invalid_storage_file_type_mapping) { - auto mapped_file_result = private_api::get_mutable_mapped_file_impl( - storage_record_pb, "mockup", api::StorageFileType::package_map); - ASSERT_FALSE(mapped_file_result.ok()); - auto it = mapped_file_result.error().message().find( - "Cannot create mutable mapped file for this file type"); - ASSERT_TRUE(it != std::string::npos) << mapped_file_result.error().message(); - - mapped_file_result = private_api::get_mutable_mapped_file_impl( - storage_record_pb, "mockup", api::StorageFileType::flag_map); - ASSERT_FALSE(mapped_file_result.ok()); - it = mapped_file_result.error().message().find( - "Cannot create mutable mapped file for this file type"); - ASSERT_TRUE(it != std::string::npos) << mapped_file_result.error().message(); -} - /// Test to lock down storage flag value update api TEST_F(AconfigStorageTest, test_boolean_flag_value_update) { - auto mapped_file_result = private_api::get_mutable_mapped_file_impl( - storage_record_pb, "mockup", api::StorageFileType::flag_val); + auto mapped_file_result = api::map_mutable_storage_file(flag_val); ASSERT_TRUE(mapped_file_result.ok()); - auto mapped_file = *mapped_file_result; + auto mapped_file = *mapped_file_result; for (int offset = 0; offset < 8; ++offset) { auto update_result = api::set_boolean_flag_value(mapped_file, offset, true); ASSERT_TRUE(update_result.ok()); @@ -146,10 +94,10 @@ TEST_F(AconfigStorageTest, test_boolean_flag_value_update) { /// Negative test to lock down the error when querying flag value out of range TEST_F(AconfigStorageTest, test_invalid_boolean_flag_value_update) { - auto mapped_file_result = private_api::get_mutable_mapped_file_impl( - storage_record_pb, "mockup", api::StorageFileType::flag_val); + auto mapped_file_result = api::map_mutable_storage_file(flag_val); ASSERT_TRUE(mapped_file_result.ok()); auto mapped_file = *mapped_file_result; + auto update_result = api::set_boolean_flag_value(mapped_file, 8, true); ASSERT_FALSE(update_result.ok()); ASSERT_EQ(update_result.error().message(), @@ -158,15 +106,14 @@ TEST_F(AconfigStorageTest, test_invalid_boolean_flag_value_update) { /// Test to lock down storage flag has server override update api TEST_F(AconfigStorageTest, test_flag_has_server_override_update) { - auto mapped_file_result = private_api::get_mutable_mapped_file_impl( - storage_record_pb, "mockup", api::StorageFileType::flag_info); + auto mapped_file_result = api::map_mutable_storage_file(flag_info); ASSERT_TRUE(mapped_file_result.ok()); auto mapped_file = *mapped_file_result; for (int offset = 0; offset < 8; ++offset) { auto update_result = api::set_flag_has_server_override( mapped_file, api::FlagValueType::Boolean, offset, true); - ASSERT_TRUE(update_result.ok()); + ASSERT_TRUE(update_result.ok()) << update_result.error(); auto ro_mapped_file = api::MappedStorageFile(); ro_mapped_file.file_ptr = mapped_file.file_ptr; ro_mapped_file.file_size = mapped_file.file_size; @@ -189,8 +136,7 @@ TEST_F(AconfigStorageTest, test_flag_has_server_override_update) { /// Test to lock down storage flag has local override update api TEST_F(AconfigStorageTest, test_flag_has_local_override_update) { - auto mapped_file_result = private_api::get_mutable_mapped_file_impl( - storage_record_pb, "mockup", api::StorageFileType::flag_info); + auto mapped_file_result = api::map_mutable_storage_file(flag_info); ASSERT_TRUE(mapped_file_result.ok()); auto mapped_file = *mapped_file_result; diff --git a/tools/aconfig/aconfig_storage_write_api/tests/storage_write_api_test.rs b/tools/aconfig/aconfig_storage_write_api/tests/storage_write_api_test.rs index a087c1ac10..367569def4 100644 --- a/tools/aconfig/aconfig_storage_write_api/tests/storage_write_api_test.rs +++ b/tools/aconfig/aconfig_storage_write_api/tests/storage_write_api_test.rs @@ -1,44 +1,17 @@ #[cfg(not(feature = "cargo"))] mod aconfig_storage_write_api_test { - use aconfig_storage_file::protos::ProtoStorageFiles; - use aconfig_storage_file::{FlagInfoBit, FlagValueType, StorageFileType}; + use aconfig_storage_file::{FlagInfoBit, FlagValueType}; use aconfig_storage_read_api::flag_info_query::find_flag_attribute; use aconfig_storage_read_api::flag_value_query::find_boolean_flag_value; use aconfig_storage_write_api::{ - mapped_file::get_mapped_file, set_boolean_flag_value, set_flag_has_local_override, + map_mutable_storage_file, set_boolean_flag_value, set_flag_has_local_override, set_flag_has_server_override, }; - use protobuf::Message; use std::fs::{self, File}; - use std::io::{Read, Write}; + use std::io::Read; use tempfile::NamedTempFile; - /// Write storage location record pb to a temp file - fn write_storage_record_file(flag_val: &str, flag_info: &str) -> NamedTempFile { - let text_proto = format!( - r#" -files {{ - version: 0 - container: "mockup" - package_map: "some_package_map" - flag_map: "some_flag_map" - flag_val: "{}" - flag_info: "{}" - timestamp: 12345 -}} -"#, - flag_val, flag_info - ); - let storage_files: ProtoStorageFiles = - protobuf::text_format::parse_from_str(&text_proto).unwrap(); - let mut binary_proto_bytes = Vec::new(); - storage_files.write_to_vec(&mut binary_proto_bytes).unwrap(); - let mut file = NamedTempFile::new().unwrap(); - file.write_all(&binary_proto_bytes).unwrap(); - file - } - /// Create temp file copy fn copy_to_temp_rw_file(source_file: &str) -> NamedTempFile { let file = NamedTempFile::new().unwrap(); @@ -66,18 +39,12 @@ files {{ /// Test to lock down flag value update api fn test_boolean_flag_value_update() { let flag_value_file = copy_to_temp_rw_file("./flag.val"); - let flag_info_file = copy_to_temp_rw_file("./flag.info"); let flag_value_path = flag_value_file.path().display().to_string(); - let flag_info_path = flag_info_file.path().display().to_string(); - let record_pb_file = write_storage_record_file(&flag_value_path, &flag_info_path); - let record_pb_path = record_pb_file.path().display().to_string(); // SAFETY: // The safety here is ensured as only this single threaded test process will // write to this file - let mut file = unsafe { - get_mapped_file(&record_pb_path, "mockup", StorageFileType::FlagVal).unwrap() - }; + let mut file = unsafe { map_mutable_storage_file(&flag_value_path).unwrap() }; for i in 0..8 { set_boolean_flag_value(&mut file, i, true).unwrap(); let value = get_boolean_flag_value_at_offset(&flag_value_path, i); @@ -92,19 +59,13 @@ files {{ #[test] /// Test to lock down flag has server override update api fn test_set_flag_has_server_override() { - let flag_value_file = copy_to_temp_rw_file("./flag.val"); let flag_info_file = copy_to_temp_rw_file("./flag.info"); - let flag_value_path = flag_value_file.path().display().to_string(); let flag_info_path = flag_info_file.path().display().to_string(); - let record_pb_file = write_storage_record_file(&flag_value_path, &flag_info_path); - let record_pb_path = record_pb_file.path().display().to_string(); // SAFETY: // The safety here is ensured as only this single threaded test process will // write to this file - let mut file = unsafe { - get_mapped_file(&record_pb_path, "mockup", StorageFileType::FlagInfo).unwrap() - }; + let mut file = unsafe { map_mutable_storage_file(&flag_info_path).unwrap() }; for i in 0..8 { set_flag_has_server_override(&mut file, FlagValueType::Boolean, i, true).unwrap(); let attribute = @@ -120,19 +81,13 @@ files {{ #[test] /// Test to lock down flag has local override update api fn test_set_flag_has_local_override() { - let flag_value_file = copy_to_temp_rw_file("./flag.val"); let flag_info_file = copy_to_temp_rw_file("./flag.info"); - let flag_value_path = flag_value_file.path().display().to_string(); let flag_info_path = flag_info_file.path().display().to_string(); - let record_pb_file = write_storage_record_file(&flag_value_path, &flag_info_path); - let record_pb_path = record_pb_file.path().display().to_string(); // SAFETY: // The safety here is ensured as only this single threaded test process will // write to this file - let mut file = unsafe { - get_mapped_file(&record_pb_path, "mockup", StorageFileType::FlagInfo).unwrap() - }; + let mut file = unsafe { map_mutable_storage_file(&flag_info_path).unwrap() }; for i in 0..8 { set_flag_has_local_override(&mut file, FlagValueType::Boolean, i, true).unwrap(); let attribute =