From 559047858042e05187085f12d9eadcc2c4e6b5e9 Mon Sep 17 00:00:00 2001 From: Dennis Shen Date: Wed, 22 May 2024 13:57:43 +0000 Subject: [PATCH] aconfig: update storage read api Update storage read api to not find storage file location from a pb file, instead directly read from /metadata copy. Previously for package.map and flag.map, we are reading from the respective RO partition. Now we are reading from /metadata/maps dir. This has a few advantages: 1, early flag availability, since /metadata can be mounted much earlier than mainline modules, so it would make mainline flags availabile even before mainline modules are mounted. 2, we no longer need to read from a pb file to find where package.map and flag.map are. Thus the read api can be further simplified and downsized. With this change, we are able to shrink the cc flag read api lib size from 171k to 120k. Bug: b/321077378 Test atest -c Change-Id: Ic9086fe4c49c139a4d1c66a8d472023a88c9dd17 --- .../aconfig_storage_read_api/Android.bp | 5 +- .../aconfig_storage_read_api/Cargo.toml | 3 +- .../aconfig_storage_read_api/src/lib.rs | 80 ++++---- .../src/mapped_file.rs | 180 +++++------------- .../src/test_utils.rs | 26 --- .../aconfig_storage_read_api/tests/Android.bp | 5 +- .../tests/storage_read_api_test.cpp | 83 ++++---- .../tests/storage_read_api_test.rs | 103 +++++----- .../aconfig_storage_write_api/Android.bp | 1 - .../aconfig_storage_write_api/Cargo.toml | 1 - .../aconfig_storage_write_api.cpp | 3 - .../tests/Android.bp | 2 - .../tests/storage_write_api_test.cpp | 2 - 13 files changed, 163 insertions(+), 331 deletions(-) delete mode 100644 tools/aconfig/aconfig_storage_read_api/src/test_utils.rs diff --git a/tools/aconfig/aconfig_storage_read_api/Android.bp b/tools/aconfig/aconfig_storage_read_api/Android.bp index db362944c5..b703b4e8f1 100644 --- a/tools/aconfig/aconfig_storage_read_api/Android.bp +++ b/tools/aconfig/aconfig_storage_read_api/Android.bp @@ -9,8 +9,6 @@ rust_defaults { srcs: ["src/lib.rs"], rustlibs: [ "libanyhow", - "libonce_cell", - "libtempfile", "libmemmap2", "libcxx", "libthiserror", @@ -34,6 +32,9 @@ rust_test_host { name: "aconfig_storage_read_api.test", test_suites: ["general-tests"], defaults: ["aconfig_storage_read_api.defaults"], + rustlibs: [ + "librand", + ], data: [ "tests/package.map", "tests/flag.map", diff --git a/tools/aconfig/aconfig_storage_read_api/Cargo.toml b/tools/aconfig/aconfig_storage_read_api/Cargo.toml index 30a4298826..2b27e4b491 100644 --- a/tools/aconfig/aconfig_storage_read_api/Cargo.toml +++ b/tools/aconfig/aconfig_storage_read_api/Cargo.toml @@ -8,10 +8,9 @@ default = ["cargo"] cargo = [] [dependencies] +rand = "0.8.5" anyhow = "1.0.69" memmap2 = "0.8.0" -once_cell = "1.19.0" -tempfile = "3.9.0" cxx = "1.0" thiserror = "1.0.56" aconfig_storage_file = { path = "../aconfig_storage_file" } diff --git a/tools/aconfig/aconfig_storage_read_api/src/lib.rs b/tools/aconfig/aconfig_storage_read_api/src/lib.rs index e4192066d5..61f9e96f84 100644 --- a/tools/aconfig/aconfig_storage_read_api/src/lib.rs +++ b/tools/aconfig/aconfig_storage_read_api/src/lib.rs @@ -42,9 +42,6 @@ pub mod flag_value_query; pub mod mapped_file; pub mod package_table_query; -#[cfg(test)] -mod test_utils; - pub use aconfig_storage_file::{AconfigStorageError, FlagValueType, StorageFileType}; pub use flag_table_query::FlagReadContext; pub use package_table_query::PackageReadContext; @@ -60,8 +57,8 @@ use memmap2::Mmap; use std::fs::File; use std::io::Read; -/// Storage file location pb file -pub const STORAGE_LOCATION_FILE: &str = "/metadata/aconfig/boot/available_storage_file_records.pb"; +/// Storage file location +pub const STORAGE_LOCATION: &str = "/metadata/aconfig"; /// Get read only mapped storage files. /// @@ -78,7 +75,7 @@ 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) } + unsafe { crate::mapped_file::get_mapped_file(STORAGE_LOCATION, container, file_type) } } /// Get package read context for a specific package. @@ -394,45 +391,41 @@ pub fn get_storage_file_version_cxx(file_path: &str) -> ffi::VersionNumberQueryC mod tests { use super::*; use crate::mapped_file::get_mapped_file; - 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::{FlagInfoBit, StoredFlagType}; - use tempfile::NamedTempFile; + use rand::Rng; + use std::fs; - fn create_test_storage_files() -> [NamedTempFile; 5] { - let package_map = copy_to_temp_file("./tests/package.map").unwrap(); - let flag_map = copy_to_temp_file("./tests/flag.map").unwrap(); - let flag_val = copy_to_temp_file("./tests/flag.val").unwrap(); - let flag_info = copy_to_temp_file("./tests/flag.info").unwrap(); + fn create_test_storage_files() -> String { + let mut rng = rand::thread_rng(); + let number: u32 = rng.gen(); + let storage_dir = String::from("/tmp/") + &number.to_string(); + if std::fs::metadata(&storage_dir).is_ok() { + fs::remove_dir_all(&storage_dir).unwrap(); + } + let maps_dir = storage_dir.clone() + "/maps"; + let boot_dir = storage_dir.clone() + "/boot"; + fs::create_dir(&storage_dir).unwrap(); + fs::create_dir(&maps_dir).unwrap(); + fs::create_dir(&boot_dir).unwrap(); - let text_proto = format!( - r#" -files {{ - version: 0 - container: "mockup" - package_map: "{}" - flag_map: "{}" - flag_val: "{}" - flag_info: "{}" - timestamp: 12345 -}} -"#, - package_map.path().display(), - flag_map.path().display(), - flag_val.path().display(), - flag_info.path().display() - ); - let pb_file = write_proto_to_temp_file(&text_proto).unwrap(); - [package_map, flag_map, flag_val, flag_info, pb_file] + let package_map = storage_dir.clone() + "/maps/mockup.package.map"; + let flag_map = storage_dir.clone() + "/maps/mockup.flag.map"; + let flag_val = storage_dir.clone() + "/boot/mockup.val"; + let flag_info = storage_dir.clone() + "/boot/mockup.info"; + fs::copy("./tests/package.map", &package_map).unwrap(); + fs::copy("./tests/flag.map", &flag_map).unwrap(); + fs::copy("./tests/flag.val", &flag_val).unwrap(); + fs::copy("./tests/flag.info", &flag_info).unwrap(); + + return storage_dir; } #[test] // this test point locks down flag package read context query fn test_package_context_query() { - let [_package_map, _flag_map, _flag_val, _flag_info, pb_file] = create_test_storage_files(); - let pb_file_path = pb_file.path().display().to_string(); + let storage_dir = create_test_storage_files(); let package_mapped_file = unsafe { - get_mapped_file(&pb_file_path, "mockup", StorageFileType::PackageMap).unwrap() + get_mapped_file(&storage_dir, "mockup", StorageFileType::PackageMap).unwrap() }; let package_context = @@ -460,10 +453,9 @@ files {{ #[test] // this test point locks down flag read context query fn test_flag_context_query() { - let [_package_map, _flag_map, _flag_val, _flag_info, pb_file] = create_test_storage_files(); - let pb_file_path = pb_file.path().display().to_string(); + let storage_dir = create_test_storage_files(); let flag_mapped_file = - unsafe { get_mapped_file(&pb_file_path, "mockup", StorageFileType::FlagMap).unwrap() }; + unsafe { get_mapped_file(&storage_dir, "mockup", StorageFileType::FlagMap).unwrap() }; let baseline = vec![ (0, "enabled_ro", StoredFlagType::ReadOnlyBoolean, 1u16), @@ -486,10 +478,9 @@ files {{ #[test] // this test point locks down flag value query fn test_flag_value_query() { - let [_package_map, _flag_map, _flag_val, _flag_info, pb_file] = create_test_storage_files(); - let pb_file_path = pb_file.path().display().to_string(); + let storage_dir = create_test_storage_files(); let flag_value_file = - unsafe { get_mapped_file(&pb_file_path, "mockup", StorageFileType::FlagVal).unwrap() }; + unsafe { get_mapped_file(&storage_dir, "mockup", StorageFileType::FlagVal).unwrap() }; let baseline: Vec = vec![false, true, true, false, true, true, true, true]; for (offset, expected_value) in baseline.into_iter().enumerate() { let flag_value = get_boolean_flag_value(&flag_value_file, offset as u32).unwrap(); @@ -500,10 +491,9 @@ files {{ #[test] // this test point locks donw flag info query fn test_flag_info_query() { - let [_package_map, _flag_map, _flag_val, _flag_info, pb_file] = create_test_storage_files(); - let pb_file_path = pb_file.path().display().to_string(); + let storage_dir = create_test_storage_files(); let flag_info_file = - unsafe { get_mapped_file(&pb_file_path, "mockup", StorageFileType::FlagInfo).unwrap() }; + unsafe { get_mapped_file(&storage_dir, "mockup", StorageFileType::FlagInfo).unwrap() }; let is_rw: Vec = vec![true, false, true, true, false, false, false, true]; for (offset, expected_value) in is_rw.into_iter().enumerate() { let attribute = diff --git a/tools/aconfig/aconfig_storage_read_api/src/mapped_file.rs b/tools/aconfig/aconfig_storage_read_api/src/mapped_file.rs index 378644317c..5a1664535f 100644 --- a/tools/aconfig/aconfig_storage_read_api/src/mapped_file.rs +++ b/tools/aconfig/aconfig_storage_read_api/src/mapped_file.rs @@ -14,47 +14,12 @@ * limitations under the License. */ -use std::fs::File; -use std::io::{BufReader, Read}; - use anyhow::anyhow; use memmap2::Mmap; +use std::fs::File; -use crate::AconfigStorageError::{ - self, FileReadFail, MapFileFail, ProtobufParseFail, StorageFileNotFound, -}; +use crate::AconfigStorageError::{self, FileReadFail, MapFileFail, StorageFileNotFound}; use crate::StorageFileType; -use aconfig_storage_file::protos::{ - storage_record_pb::try_from_binary_proto, ProtoStorageFileInfo, ProtoStorageFiles, -}; - -/// Find where storage files are stored for a particular container -pub fn find_container_storage_location( - location_pb_file: &str, - container: &str, -) -> Result { - let file = File::open(location_pb_file).map_err(|errmsg| { - FileReadFail(anyhow!("Failed to open file {}: {}", location_pb_file, errmsg)) - })?; - let mut reader = BufReader::new(file); - let mut bytes = Vec::new(); - reader.read_to_end(&mut bytes).map_err(|errmsg| { - FileReadFail(anyhow!("Failed to read file {}: {}", location_pb_file, errmsg)) - })?; - let storage_locations: ProtoStorageFiles = try_from_binary_proto(&bytes).map_err(|errmsg| { - ProtobufParseFail(anyhow!( - "Failed to parse storage location pb file {}: {}", - location_pb_file, - errmsg - )) - })?; - for location_info in storage_locations.files.iter() { - if location_info.container() == container { - return Ok(location_info.clone()); - } - } - Err(StorageFileNotFound(anyhow!("Storage file does not exist for {}", container))) -} /// Get the read only memory mapping of a storage file /// @@ -82,123 +47,70 @@ unsafe fn map_file(file_path: &str) -> Result { /// file after being mapped. Ensure no writes can happen to this file while this /// mapping stays alive. pub unsafe fn get_mapped_file( - location_pb_file: &str, + storage_dir: &str, container: &str, file_type: StorageFileType, ) -> Result { - let files_location = find_container_storage_location(location_pb_file, container)?; - match file_type { - StorageFileType::PackageMap => unsafe { map_file(files_location.package_map()) }, - StorageFileType::FlagMap => unsafe { map_file(files_location.flag_map()) }, - StorageFileType::FlagVal => unsafe { map_file(files_location.flag_val()) }, - StorageFileType::FlagInfo => unsafe { map_file(files_location.flag_info()) }, + let storage_file = match file_type { + StorageFileType::PackageMap => { + String::from(storage_dir) + "/maps/" + container + ".package.map" + } + StorageFileType::FlagMap => String::from(storage_dir) + "/maps/" + container + ".flag.map", + StorageFileType::FlagVal => String::from(storage_dir) + "/boot/" + container + ".val", + StorageFileType::FlagInfo => String::from(storage_dir) + "/boot/" + container + ".info", + }; + if std::fs::metadata(&storage_file).is_err() { + return Err(StorageFileNotFound(anyhow!("storage file {} does not exist", storage_file))); } + unsafe { map_file(&storage_file) } } #[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 tempfile::NamedTempFile; + use rand::Rng; + use std::fs; + use std::io::Read; - #[test] - fn test_find_storage_file_location() { - let text_proto = r#" -files { - version: 0 - container: "system" - package_map: "/system/etc/package.map" - flag_map: "/system/etc/flag.map" - flag_val: "/metadata/aconfig/system.val" - timestamp: 12345 -} -files { - version: 1 - container: "product" - package_map: "/product/etc/package.map" - flag_map: "/product/etc/flag.map" - flag_val: "/metadata/aconfig/product.val" - timestamp: 54321 -} -"#; - let file = write_proto_to_temp_file(&text_proto).unwrap(); - let file_full_path = file.path().display().to_string(); - let file_info = find_container_storage_location(&file_full_path, "system").unwrap(); - assert_eq!(file_info.version(), 0); - assert_eq!(file_info.container(), "system"); - assert_eq!(file_info.package_map(), "/system/etc/package.map"); - assert_eq!(file_info.flag_map(), "/system/etc/flag.map"); - assert_eq!(file_info.flag_val(), "/metadata/aconfig/system.val"); - assert_eq!(file_info.timestamp(), 12345); - - let file_info = find_container_storage_location(&file_full_path, "product").unwrap(); - assert_eq!(file_info.version(), 1); - assert_eq!(file_info.container(), "product"); - assert_eq!(file_info.package_map(), "/product/etc/package.map"); - assert_eq!(file_info.flag_map(), "/product/etc/flag.map"); - assert_eq!(file_info.flag_val(), "/metadata/aconfig/product.val"); - assert_eq!(file_info.timestamp(), 54321); - - let err = find_container_storage_location(&file_full_path, "vendor").unwrap_err(); - assert_eq!( - format!("{:?}", err), - "StorageFileNotFound(Storage file does not exist for vendor)" - ); - } - - fn map_and_verify(location_pb_file: &str, file_type: StorageFileType, actual_file: &str) { + fn map_and_verify(storage_dir: &str, file_type: StorageFileType, actual_file: &str) { let mut opened_file = File::open(actual_file).unwrap(); let mut content = Vec::new(); opened_file.read_to_end(&mut content).unwrap(); - - let mmaped_file = - unsafe { get_mapped_file(location_pb_file, "system", file_type).unwrap() }; + let mmaped_file = unsafe { get_mapped_file(storage_dir, "mockup", file_type).unwrap() }; assert_eq!(mmaped_file[..], content[..]); } - fn create_test_storage_files() -> [NamedTempFile; 4] { - let package_map = copy_to_temp_file("./tests/package.map").unwrap(); - let flag_map = copy_to_temp_file("./tests/flag.map").unwrap(); - let flag_val = copy_to_temp_file("./tests/package.map").unwrap(); + fn create_test_storage_files() -> String { + let mut rng = rand::thread_rng(); + let number: u32 = rng.gen(); + let storage_dir = String::from("/tmp/") + &number.to_string(); + if std::fs::metadata(&storage_dir).is_ok() { + fs::remove_dir_all(&storage_dir).unwrap(); + } + let maps_dir = storage_dir.clone() + "/maps"; + let boot_dir = storage_dir.clone() + "/boot"; + fs::create_dir(&storage_dir).unwrap(); + fs::create_dir(&maps_dir).unwrap(); + fs::create_dir(&boot_dir).unwrap(); - let text_proto = format!( - r#" -files {{ - version: 0 - container: "system" - package_map: "{}" - flag_map: "{}" - flag_val: "{}" - timestamp: 12345 -}} -"#, - package_map.path().display(), - flag_map.path().display(), - flag_val.path().display() - ); - let pb_file = write_proto_to_temp_file(&text_proto).unwrap(); - [package_map, flag_map, flag_val, pb_file] + let package_map = storage_dir.clone() + "/maps/mockup.package.map"; + let flag_map = storage_dir.clone() + "/maps/mockup.flag.map"; + let flag_val = storage_dir.clone() + "/boot/mockup.val"; + let flag_info = storage_dir.clone() + "/boot/mockup.info"; + fs::copy("./tests/package.map", &package_map).unwrap(); + fs::copy("./tests/flag.map", &flag_map).unwrap(); + fs::copy("./tests/flag.val", &flag_val).unwrap(); + fs::copy("./tests/flag.info", &flag_info).unwrap(); + + return storage_dir; } #[test] fn test_mapped_file_contents() { - let [package_map, flag_map, flag_val, pb_file] = create_test_storage_files(); - let pb_file_path = pb_file.path().display().to_string(); - map_and_verify( - &pb_file_path, - StorageFileType::PackageMap, - &package_map.path().display().to_string(), - ); - map_and_verify( - &pb_file_path, - StorageFileType::FlagMap, - &flag_map.path().display().to_string(), - ); - map_and_verify( - &pb_file_path, - StorageFileType::FlagVal, - &flag_val.path().display().to_string(), - ); + let storage_dir = create_test_storage_files(); + map_and_verify(&storage_dir, StorageFileType::PackageMap, "./tests/package.map"); + map_and_verify(&storage_dir, StorageFileType::FlagMap, "./tests/flag.map"); + map_and_verify(&storage_dir, StorageFileType::FlagVal, "./tests/flag.val"); + map_and_verify(&storage_dir, StorageFileType::FlagInfo, "./tests/flag.info"); } } diff --git a/tools/aconfig/aconfig_storage_read_api/src/test_utils.rs b/tools/aconfig/aconfig_storage_read_api/src/test_utils.rs deleted file mode 100644 index 84f31aa710..0000000000 --- a/tools/aconfig/aconfig_storage_read_api/src/test_utils.rs +++ /dev/null @@ -1,26 +0,0 @@ -/* - * Copyright (C) 2023 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -use anyhow::Result; -use std::fs; -use tempfile::NamedTempFile; - -/// Create temp file copy -pub(crate) fn copy_to_temp_file(source_file: &str) -> Result { - let file = NamedTempFile::new()?; - fs::copy(source_file, file.path())?; - Ok(file) -} diff --git a/tools/aconfig/aconfig_storage_read_api/tests/Android.bp b/tools/aconfig/aconfig_storage_read_api/tests/Android.bp index 6b05ca6fb1..98944d60dc 100644 --- a/tools/aconfig/aconfig_storage_read_api/tests/Android.bp +++ b/tools/aconfig/aconfig_storage_read_api/tests/Android.bp @@ -7,8 +7,7 @@ rust_test { "libanyhow", "libaconfig_storage_file", "libaconfig_storage_read_api", - "libprotobuf", - "libtempfile", + "librand", ], data: [ "package.map", @@ -26,8 +25,6 @@ cc_test { ], static_libs: [ "libgmock", - "libaconfig_storage_protos_cc", - "libprotobuf-cpp-lite", "libaconfig_storage_read_api_cc", "libbase", "liblog", 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 5393c49a7d..86a0541c53 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 @@ -22,11 +22,9 @@ #include #include "aconfig_storage/aconfig_storage_read_api.hpp" #include -#include #include #include -using android::aconfig_storage_metadata::storage_files; using namespace android::base; namespace api = aconfig_storage; @@ -34,49 +32,33 @@ namespace private_api = aconfig_storage::private_internal_api; class AconfigStorageTest : public ::testing::Test { protected: - Result copy_to_temp_file(std::string const& source_file) { - auto temp_file = std::string(std::tmpnam(nullptr)); + Result copy_file(std::string const& src_file, + std::string const& dst_file) { auto content = std::string(); - if (!ReadFileToString(source_file, &content)) { - return Error() << "failed to read file: " << source_file; + if (!ReadFileToString(src_file, &content)) { + return Error() << "failed to read file: " << src_file; } - if (!WriteStringToFile(content, temp_file)) { - return Error() << "failed to copy file: " << source_file; + if (!WriteStringToFile(content, dst_file)) { + return Error() << "failed to copy file: " << dst_file; } - return temp_file; - } - - Result write_storage_location_pb_file(std::string const& package_map, - std::string const& flag_map, - 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(package_map); - info->set_flag_map(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; + return {}; } void SetUp() override { auto const test_dir = android::base::GetExecutableDirectory(); - package_map = *copy_to_temp_file(test_dir + "/package.map"); - flag_map = *copy_to_temp_file(test_dir + "/flag.map"); - flag_val = *copy_to_temp_file(test_dir + "/flag.val"); - flag_info = *copy_to_temp_file(test_dir + "/flag.info"); - storage_record_pb = *write_storage_location_pb_file( - package_map, flag_map, flag_val, flag_info); + storage_dir = std::string(root_dir.path); + auto maps_dir = storage_dir + "/maps"; + auto boot_dir = storage_dir + "/boot"; + mkdir(maps_dir.c_str(), 0775); + mkdir(boot_dir.c_str(), 0775); + package_map = std::string(maps_dir) + "/mockup.package.map"; + flag_map = std::string(maps_dir) + "/mockup.flag.map"; + flag_val = std::string(boot_dir) + "/mockup.val"; + flag_info = std::string(boot_dir) + "/mockup.info"; + copy_file(test_dir + "/package.map", package_map); + copy_file(test_dir + "/flag.map", flag_map); + copy_file(test_dir + "/flag.val", flag_val); + copy_file(test_dir + "/flag.info", flag_info); } void TearDown() override { @@ -84,14 +66,14 @@ class AconfigStorageTest : public ::testing::Test { std::remove(flag_map.c_str()); std::remove(flag_val.c_str()); std::remove(flag_info.c_str()); - std::remove(storage_record_pb.c_str()); } + TemporaryDir root_dir; + std::string storage_dir; std::string package_map; std::string flag_map; std::string flag_val; std::string flag_info; - std::string storage_record_pb; }; /// Test to lock down storage file version query api @@ -113,16 +95,17 @@ TEST_F(AconfigStorageTest, test_storage_version_query) { /// 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_mapped_file_impl( - storage_record_pb, "vendor", api::StorageFileType::package_map); + storage_dir, "vendor", api::StorageFileType::package_map); ASSERT_FALSE(mapped_file_result.ok()); ASSERT_EQ(mapped_file_result.error().message(), - "Unable to find storage files for container vendor"); + std::string("failed to open ") + storage_dir + + "/maps/vendor.package.map: No such file or directory"); } /// Test to lock down storage package context query api TEST_F(AconfigStorageTest, test_package_context_query) { auto mapped_file_result = private_api::get_mapped_file_impl( - storage_record_pb, "mockup", api::StorageFileType::package_map); + storage_dir, "mockup", api::StorageFileType::package_map); ASSERT_TRUE(mapped_file_result.ok()); auto mapped_file = std::unique_ptr(*mapped_file_result); @@ -151,7 +134,7 @@ TEST_F(AconfigStorageTest, test_package_context_query) { /// Test to lock down when querying none exist package TEST_F(AconfigStorageTest, test_none_existent_package_context_query) { auto mapped_file_result = private_api::get_mapped_file_impl( - storage_record_pb, "mockup", api::StorageFileType::package_map); + storage_dir, "mockup", api::StorageFileType::package_map); ASSERT_TRUE(mapped_file_result.ok()); auto mapped_file = std::unique_ptr(*mapped_file_result); @@ -164,7 +147,7 @@ TEST_F(AconfigStorageTest, test_none_existent_package_context_query) { /// Test to lock down storage flag context query api TEST_F(AconfigStorageTest, test_flag_context_query) { auto mapped_file_result = private_api::get_mapped_file_impl( - storage_record_pb, "mockup", api::StorageFileType::flag_map); + storage_dir, "mockup", api::StorageFileType::flag_map); ASSERT_TRUE(mapped_file_result.ok()); auto mapped_file = std::unique_ptr(*mapped_file_result); @@ -190,7 +173,7 @@ TEST_F(AconfigStorageTest, test_flag_context_query) { /// Test to lock down when querying none exist flag TEST_F(AconfigStorageTest, test_none_existent_flag_context_query) { auto mapped_file_result = private_api::get_mapped_file_impl( - storage_record_pb, "mockup", api::StorageFileType::flag_map); + storage_dir, "mockup", api::StorageFileType::flag_map); ASSERT_TRUE(mapped_file_result.ok()); auto mapped_file = std::unique_ptr(*mapped_file_result); @@ -206,7 +189,7 @@ TEST_F(AconfigStorageTest, test_none_existent_flag_context_query) { /// Test to lock down storage flag value query api TEST_F(AconfigStorageTest, test_boolean_flag_value_query) { auto mapped_file_result = private_api::get_mapped_file_impl( - storage_record_pb, "mockup", api::StorageFileType::flag_val); + storage_dir, "mockup", api::StorageFileType::flag_val); ASSERT_TRUE(mapped_file_result.ok()); auto mapped_file = std::unique_ptr(*mapped_file_result); @@ -222,7 +205,7 @@ TEST_F(AconfigStorageTest, test_boolean_flag_value_query) { /// Negative test to lock down the error when querying flag value out of range TEST_F(AconfigStorageTest, test_invalid_boolean_flag_value_query) { auto mapped_file_result = private_api::get_mapped_file_impl( - storage_record_pb, "mockup", api::StorageFileType::flag_val); + storage_dir, "mockup", api::StorageFileType::flag_val); ASSERT_TRUE(mapped_file_result.ok()); auto mapped_file = std::unique_ptr(*mapped_file_result); @@ -235,7 +218,7 @@ TEST_F(AconfigStorageTest, test_invalid_boolean_flag_value_query) { /// Test to lock down storage flag info query api TEST_F(AconfigStorageTest, test_boolean_flag_info_query) { auto mapped_file_result = private_api::get_mapped_file_impl( - storage_record_pb, "mockup", api::StorageFileType::flag_info); + storage_dir, "mockup", api::StorageFileType::flag_info); ASSERT_TRUE(mapped_file_result.ok()); auto mapped_file = std::unique_ptr(*mapped_file_result); @@ -254,7 +237,7 @@ TEST_F(AconfigStorageTest, test_boolean_flag_info_query) { /// Negative test to lock down the error when querying flag info out of range TEST_F(AconfigStorageTest, test_invalid_boolean_flag_info_query) { auto mapped_file_result = private_api::get_mapped_file_impl( - storage_record_pb, "mockup", api::StorageFileType::flag_info); + storage_dir, "mockup", api::StorageFileType::flag_info); ASSERT_TRUE(mapped_file_result.ok()); auto mapped_file = std::unique_ptr(*mapped_file_result); 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 ecba573d82..afc44d4d70 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 @@ -1,71 +1,63 @@ #[cfg(not(feature = "cargo"))] mod aconfig_storage_rust_test { - use aconfig_storage_file::protos::storage_record_pb::write_proto_to_temp_file; use aconfig_storage_file::{FlagInfoBit, FlagValueType, StorageFileType, StoredFlagType}; use aconfig_storage_read_api::{ get_boolean_flag_value, get_flag_attribute, get_flag_read_context, get_package_read_context, get_storage_file_version, mapped_file::get_mapped_file, PackageReadContext, }; + use rand::Rng; use std::fs; - use tempfile::NamedTempFile; - pub fn copy_to_temp_file(source_file: &str) -> NamedTempFile { - let file = NamedTempFile::new().unwrap(); - fs::copy(source_file, file.path()).unwrap(); - file - } + fn create_test_storage_files() -> String { + let mut rng = rand::thread_rng(); + let number: u32 = rng.gen(); + let storage_dir = String::from("/tmp/") + &number.to_string(); + if std::fs::metadata(&storage_dir).is_ok() { + fs::remove_dir_all(&storage_dir).unwrap(); + } + let maps_dir = storage_dir.clone() + "/maps"; + let boot_dir = storage_dir.clone() + "/boot"; + fs::create_dir(&storage_dir).unwrap(); + fs::create_dir(maps_dir).unwrap(); + fs::create_dir(boot_dir).unwrap(); - fn create_test_storage_files() -> [NamedTempFile; 5] { - let package_map = copy_to_temp_file("./package.map"); - let flag_map = copy_to_temp_file("./flag.map"); - let flag_val = copy_to_temp_file("./flag.val"); - let flag_info = copy_to_temp_file("./flag.info"); + let package_map = storage_dir.clone() + "/maps/mockup.package.map"; + let flag_map = storage_dir.clone() + "/maps/mockup.flag.map"; + let flag_val = storage_dir.clone() + "/boot/mockup.val"; + let flag_info = storage_dir.clone() + "/boot/mockup.info"; + fs::copy("./package.map", package_map).unwrap(); + fs::copy("./flag.map", flag_map).unwrap(); + fs::copy("./flag.val", flag_val).unwrap(); + fs::copy("./flag.info", flag_info).unwrap(); - let text_proto = format!( - r#" -files {{ - version: 0 - container: "mockup" - package_map: "{}" - flag_map: "{}" - flag_val: "{}" - flag_info: "{}" - timestamp: 12345 -}} -"#, - package_map.path().display(), - flag_map.path().display(), - flag_val.path().display(), - flag_info.path().display() - ); - let pb_file = write_proto_to_temp_file(&text_proto).unwrap(); - [package_map, flag_map, flag_val, flag_info, pb_file] + storage_dir } #[test] fn test_unavailable_stoarge() { - let [_package_map, _flag_map, _flag_val, _flag_info, pb_file] = create_test_storage_files(); - let pb_file_path = pb_file.path().display().to_string(); + let storage_dir = create_test_storage_files(); // SAFETY: // The safety here is ensured as the test process will not write to temp storage file let err = unsafe { - get_mapped_file(&pb_file_path, "vendor", StorageFileType::PackageMap).unwrap_err() + get_mapped_file(&storage_dir, "vendor", StorageFileType::PackageMap).unwrap_err() }; assert_eq!( format!("{:?}", err), - "StorageFileNotFound(Storage file does not exist for vendor)" + format!( + "StorageFileNotFound(storage file {}/maps/vendor.package.map does not exist)", + storage_dir + ) ); } #[test] fn test_package_context_query() { - let [_package_map, _flag_map, _flag_val, _flag_info, pb_file] = create_test_storage_files(); - let pb_file_path = pb_file.path().display().to_string(); + let storage_dir = create_test_storage_files(); // SAFETY: // The safety here is ensured as the test process will not write to temp storage file let package_mapped_file = unsafe { - get_mapped_file(&pb_file_path, "mockup", StorageFileType::PackageMap).unwrap() + get_mapped_file(&storage_dir, "mockup", StorageFileType::PackageMap).unwrap() }; let package_context = @@ -92,12 +84,11 @@ files {{ #[test] fn test_none_exist_package_context_query() { - let [_package_map, _flag_map, _flag_val, _flag_info, pb_file] = create_test_storage_files(); - let pb_file_path = pb_file.path().display().to_string(); + let storage_dir = create_test_storage_files(); // SAFETY: // The safety here is ensured as the test process will not write to temp storage file let package_mapped_file = unsafe { - get_mapped_file(&pb_file_path, "mockup", StorageFileType::PackageMap).unwrap() + get_mapped_file(&storage_dir, "mockup", StorageFileType::PackageMap).unwrap() }; let package_context_option = @@ -108,12 +99,11 @@ files {{ #[test] fn test_flag_context_query() { - let [_package_map, _flag_map, _flag_val, _flag_info, pb_file] = create_test_storage_files(); - let pb_file_path = pb_file.path().display().to_string(); + let storage_dir = create_test_storage_files(); // SAFETY: // The safety here is ensured as the test process will not write to temp storage file let flag_mapped_file = - unsafe { get_mapped_file(&pb_file_path, "mockup", StorageFileType::FlagMap).unwrap() }; + unsafe { get_mapped_file(&storage_dir, "mockup", StorageFileType::FlagMap).unwrap() }; let baseline = vec![ (0, "enabled_ro", StoredFlagType::ReadOnlyBoolean, 1u16), @@ -135,12 +125,11 @@ files {{ #[test] fn test_none_exist_flag_context_query() { - let [_package_map, _flag_map, _flag_val, _flag_info, pb_file] = create_test_storage_files(); - let pb_file_path = pb_file.path().display().to_string(); + let storage_dir = create_test_storage_files(); // SAFETY: // The safety here is ensured as the test process will not write to temp storage file let flag_mapped_file = - unsafe { get_mapped_file(&pb_file_path, "mockup", StorageFileType::FlagMap).unwrap() }; + unsafe { get_mapped_file(&storage_dir, "mockup", StorageFileType::FlagMap).unwrap() }; let flag_context_option = get_flag_read_context(&flag_mapped_file, 0, "none_exist").unwrap(); assert_eq!(flag_context_option, None); @@ -152,12 +141,11 @@ files {{ #[test] fn test_boolean_flag_value_query() { - let [_package_map, _flag_map, _flag_val, _flag_info, pb_file] = create_test_storage_files(); - let pb_file_path = pb_file.path().display().to_string(); + let storage_dir = create_test_storage_files(); // SAFETY: // The safety here is ensured as the test process will not write to temp storage file let flag_value_file = - unsafe { get_mapped_file(&pb_file_path, "mockup", StorageFileType::FlagVal).unwrap() }; + unsafe { get_mapped_file(&storage_dir, "mockup", StorageFileType::FlagVal).unwrap() }; let baseline: Vec = vec![false, true, true, false, true, true, true, true]; for (offset, expected_value) in baseline.into_iter().enumerate() { let flag_value = get_boolean_flag_value(&flag_value_file, offset as u32).unwrap(); @@ -167,12 +155,11 @@ files {{ #[test] fn test_invalid_boolean_flag_value_query() { - let [_package_map, _flag_map, _flag_val, _flag_info, pb_file] = create_test_storage_files(); - let pb_file_path = pb_file.path().display().to_string(); + let storage_dir = create_test_storage_files(); // SAFETY: // The safety here is ensured as the test process will not write to temp storage file let flag_value_file = - unsafe { get_mapped_file(&pb_file_path, "mockup", StorageFileType::FlagVal).unwrap() }; + unsafe { get_mapped_file(&storage_dir, "mockup", StorageFileType::FlagVal).unwrap() }; let err = get_boolean_flag_value(&flag_value_file, 8u32).unwrap_err(); assert_eq!( format!("{:?}", err), @@ -182,12 +169,11 @@ files {{ #[test] fn test_flag_info_query() { - let [_package_map, _flag_map, _flag_val, _flag_info, pb_file] = create_test_storage_files(); - let pb_file_path = pb_file.path().display().to_string(); + let storage_dir = create_test_storage_files(); // SAFETY: // The safety here is ensured as the test process will not write to temp storage file let flag_info_file = - unsafe { get_mapped_file(&pb_file_path, "mockup", StorageFileType::FlagInfo).unwrap() }; + unsafe { get_mapped_file(&storage_dir, "mockup", StorageFileType::FlagInfo).unwrap() }; let is_rw: Vec = vec![true, false, true, true, false, false, false, true]; for (offset, expected_value) in is_rw.into_iter().enumerate() { let attribute = @@ -200,12 +186,11 @@ files {{ #[test] fn test_invalid_boolean_flag_info_query() { - let [_package_map, _flag_map, _flag_val, _flag_info, pb_file] = create_test_storage_files(); - let pb_file_path = pb_file.path().display().to_string(); + let storage_dir = create_test_storage_files(); // SAFETY: // The safety here is ensured as the test process will not write to temp storage file let flag_info_file = - unsafe { get_mapped_file(&pb_file_path, "mockup", StorageFileType::FlagInfo).unwrap() }; + unsafe { get_mapped_file(&storage_dir, "mockup", StorageFileType::FlagInfo).unwrap() }; let err = get_flag_attribute(&flag_info_file, FlagValueType::Boolean, 8u32).unwrap_err(); assert_eq!( format!("{:?}", err), diff --git a/tools/aconfig/aconfig_storage_write_api/Android.bp b/tools/aconfig/aconfig_storage_write_api/Android.bp index 4dbdbbfb2f..0f1962c3ac 100644 --- a/tools/aconfig/aconfig_storage_write_api/Android.bp +++ b/tools/aconfig/aconfig_storage_write_api/Android.bp @@ -77,7 +77,6 @@ cc_library_static { export_include_dirs: ["include"], static_libs: [ "libaconfig_storage_read_api_cc", - "libaconfig_storage_protos_cc", "libprotobuf-cpp-lite", "libbase", ], diff --git a/tools/aconfig/aconfig_storage_write_api/Cargo.toml b/tools/aconfig/aconfig_storage_write_api/Cargo.toml index eaa55f2531..2ce6edfe96 100644 --- a/tools/aconfig/aconfig_storage_write_api/Cargo.toml +++ b/tools/aconfig/aconfig_storage_write_api/Cargo.toml @@ -13,7 +13,6 @@ cxx = "1.0" memmap2 = "0.8.0" tempfile = "3.9.0" thiserror = "1.0.56" -protobuf = "3.2.0" aconfig_storage_file = { path = "../aconfig_storage_file" } aconfig_storage_read_api = { path = "../aconfig_storage_read_api" } 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 f529f7954c..b441e057ed 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 @@ -1,7 +1,6 @@ #include #include -#include #include #include @@ -11,8 +10,6 @@ #include "aconfig_storage/lib.rs.h" #include "aconfig_storage/aconfig_storage_write_api.hpp" -using storage_records_pb = android::aconfig_storage_metadata::storage_files; -using storage_record_pb = android::aconfig_storage_metadata::storage_file_info; using namespace android::base; namespace aconfig_storage { diff --git a/tools/aconfig/aconfig_storage_write_api/tests/Android.bp b/tools/aconfig/aconfig_storage_write_api/tests/Android.bp index 85568e063b..f6409b7090 100644 --- a/tools/aconfig/aconfig_storage_write_api/tests/Android.bp +++ b/tools/aconfig/aconfig_storage_write_api/tests/Android.bp @@ -25,8 +25,6 @@ cc_test { ], static_libs: [ "libgmock", - "libaconfig_storage_protos_cc", - "libprotobuf-cpp-lite", "libaconfig_storage_read_api_cc", "libaconfig_storage_write_api_cc", "libbase", 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 54373798c9..31183fa01b 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 @@ -22,11 +22,9 @@ #include "aconfig_storage/aconfig_storage_read_api.hpp" #include "aconfig_storage/aconfig_storage_write_api.hpp" #include -#include #include #include -using android::aconfig_storage_metadata::storage_files; using namespace android::base; namespace api = aconfig_storage;