Merge "aconfig: update aconfig storage write api and test update" into main

tools/aconfig/aconfig/src/commands.rs

@@ -239,10 +239,8 @@ pub fn create_storage(
     container: &str,
     file: &StorageFileType,
 ) -> Result<Vec<u8>> {
-    let parsed_flags_vec: Vec<ProtoParsedFlags> = caches
+    let parsed_flags_vec: Vec<ProtoParsedFlags> =
-        .into_iter()
+        caches.into_iter().map(|mut input| input.try_parse_flags()).collect::<Result<Vec<_>>>()?;
-        .map(|mut input| input.try_parse_flags())
-        .collect::<Result<Vec<_>>>()?;
     generate_storage_file(container, parsed_flags_vec.iter(), file)
 }
 

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);
     }

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"

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"

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"

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"

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"),
+                String::from("disabled_rw"),
-                StoredFlagType::ReadOnlyBoolean,
+                StoredFlagType::ReadWriteBoolean,
                 false,
             ),
             (
@@ -381,8 +381,8 @@ mod tests {
             ),
             (
                 String::from("com.android.aconfig.storage.test_4"),
-                String::from("enabled_ro"),
+                String::from("enabled_rw"),
-                StoredFlagType::ReadOnlyBoolean,
+                StoredFlagType::ReadWriteBoolean,
                 true,
             ),
         ];

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_rw", 0, 1, None),
-        FlagTableNode::new_expected(2, "enabled_ro", 1, 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 }
 }

tools/aconfig/aconfig_storage_read_api/aconfig_storage_read_api.cpp

@@ -65,8 +65,24 @@ static Result<std::string> find_storage_file(
   return Error() << "Unable to find storage files for container " << container;;
 }
 
+namespace private_internal_api {
+
+/// Get mapped file implementation.
+Result<MappedStorageFile> 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<MappedStorageFile> map_storage_file(std::string const& file) {
+Result<MappedStorageFile> 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<MappedStorageFile> map_storage_file(std::string const& file) {
   return mapped_file;
 }
 
-namespace private_internal_api {
-
-/// Get mapped file implementation.
-Result<MappedStorageFile> 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<FlagValueType> map_to_flag_value_type(
     StoredFlagType stored_type) {

tools/aconfig/aconfig_storage_read_api/include/aconfig_storage/aconfig_storage_read_api.hpp

@@ -67,6 +67,11 @@ android::base::Result<MappedStorageFile> get_mapped_file_impl(
 
 } // namespace private_internal_api
 
+/// Map a storage file
+android::base::Result<MappedStorageFile> 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

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<bool> = vec![true, false, true, false, false, false, false, false];
+        let baseline: Vec<bool> = 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();

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_rw", StoredFlagType::ReadWriteBoolean, 1u16),
-            (2, "enabled_ro", StoredFlagType::ReadOnlyBoolean, 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),

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_rw", StoredFlagType::ReadWriteBoolean, 1u16),
-            (2, "enabled_ro", StoredFlagType::ReadOnlyBoolean, 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),

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<std::tuple<int, std::string, api::StoredFlagType, int>>{
     {0, "enabled_ro", api::StoredFlagType::ReadOnlyBoolean, 1},
     {0, "enabled_rw", api::StoredFlagType::ReadWriteBoolean, 2},
-    {1, "disabled_ro", api::StoredFlagType::ReadOnlyBoolean, 0},
+    {2, "enabled_rw", api::StoredFlagType::ReadWriteBoolean, 1},
-    {2, "enabled_ro", api::StoredFlagType::ReadOnlyBoolean, 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},

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_rw", StoredFlagType::ReadWriteBoolean, 1u16),
-            (2, "enabled_ro", StoredFlagType::ReadOnlyBoolean, 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),

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<storage_records_pb> 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<std::string> 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<MutableMappedStorageFile> 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<MutableMappedStorageFile> map_mutable_storage_file(std::string const& file) {
   struct stat file_stat;
@@ -120,14 +48,6 @@ Result<MutableMappedStorageFile> map_mutable_storage_file(std::string const& fil
   return mapped_file;
 }
 
-/// Get mutable mapped file
-Result<MutableMappedStorageFile> 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<void> set_boolean_flag_value(
     const MutableMappedStorageFile& file,

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<MutableMappedStorageFile> 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<MutableMappedStorageFile> map_mutable_storage_file(
     std::string const& file);
 
-/// Get mapped writeable storage file
-Result<MutableMappedStorageFile> get_mutable_mapped_file(
-    std::string const& container,
-    StorageFileType file_type);
-
 /// Set boolean flag value
 Result<void> set_boolean_flag_value(
     const MutableMappedStorageFile& file,

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_path: path to the storage file
-/// \input file_type: storage file type enum
-/// \return a result of read write mapped 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(
+pub unsafe fn map_mutable_storage_file(file_path: &str) -> Result<MmapMut, AconfigStorageError> {
-    container: &str,
+    crate::mapped_file::map_file(file_path)
-    file_type: StorageFileType,
-) -> Result<MmapMut, AconfigStorageError> {
-    unsafe { crate::mapped_file::get_mapped_file(STORAGE_LOCATION_FILE, container, file_type) }
 }
 
 /// 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(
+            let mut file = map_mutable_storage_file(&flag_value_path).unwrap();
-                &record_pb_path,
-                "system",
-                StorageFileType::FlagVal,
-            )
-            .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(
+            let mut file = map_mutable_storage_file(&flag_info_path).unwrap();
-                &record_pb_path,
-                "system",
-                StorageFileType::FlagInfo,
-            )
-            .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(
+            let mut file = map_mutable_storage_file(&flag_info_path).unwrap();
-                &record_pb_path,
-                "system",
-                StorageFileType::FlagInfo,
-            )
-            .unwrap();
             for i in 0..8 {
                 set_flag_has_local_override(&mut file, FlagValueType::Boolean, i, true).unwrap();
                 let attribute =

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<MmapMut, AconfigStorageError> {
+pub(crate) unsafe fn map_file(file_path: &str) -> Result<MmapMut, AconfigStorageError> {
     // 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<MmapMut, AconfigStorageError> {
     }
 }
 
-/// 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<MmapMut, AconfigStorageError> {
-    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!(
+        let flag_val = rw_val_file.path().display().to_string();
-            r#"
+        let flag_info = rw_info_file.path().display().to_string();
-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 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 =
+            let mmaped_file = map_file(&flag_val).unwrap();
-                get_mapped_file(&storage_record_file_path, "system", StorageFileType::FlagVal)
-                    .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 =
+            let mmaped_file = map_file(&flag_info).unwrap();
-                get_mapped_file(&storage_record_file_path, "system", StorageFileType::FlagInfo)
-                    .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 ro_val_file = copy_to_temp_file("./tests/flag.val", true).unwrap();
-        let text_proto = format!(
+        let flag_val = ro_val_file.path().display().to_string();
-            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();
 
         // SAFETY:
         // The safety here is guaranteed here as no writes happens to this temp file
         unsafe {
-            let error =
+            let error = map_file(&flag_val).unwrap_err();
-                get_mapped_file(&storage_record_file_path, "system", StorageFileType::FlagVal)
-                    .unwrap_err();
             assert_eq!(
                 format!("{:?}", error),
-                format!(
+                format!("MapFileFail(fail to map non read write storage file {})", flag_val)
-                    "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
-                )
             );
         }
     }

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<std::string> 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(
+  auto mapped_file_result = api::map_mutable_storage_file(flag_val);
-      storage_record_pb, "mockup", api::StorageFileType::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(
+  auto mapped_file_result = api::map_mutable_storage_file(flag_val);
-      storage_record_pb, "mockup", api::StorageFileType::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(
+  auto mapped_file_result = api::map_mutable_storage_file(flag_val);
-      storage_record_pb, "mockup", api::StorageFileType::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(
+  auto mapped_file_result = api::map_mutable_storage_file(flag_info);
-      storage_record_pb, "mockup", api::StorageFileType::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(
+  auto mapped_file_result = api::map_mutable_storage_file(flag_info);
-      storage_record_pb, "mockup", api::StorageFileType::flag_info);
   ASSERT_TRUE(mapped_file_result.ok());
   auto mapped_file = *mapped_file_result;
 

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};
-    use aconfig_storage_file::{FlagInfoBit, FlagValueType, StorageFileType};
     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 {
+        let mut file = unsafe { map_mutable_storage_file(&flag_value_path).unwrap() };
-            get_mapped_file(&record_pb_path, "mockup", StorageFileType::FlagVal).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 {
+        let mut file = unsafe { map_mutable_storage_file(&flag_info_path).unwrap() };
-            get_mapped_file(&record_pb_path, "mockup", StorageFileType::FlagInfo).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 {
+        let mut file = unsafe { map_mutable_storage_file(&flag_info_path).unwrap() };
-            get_mapped_file(&record_pb_path, "mockup", StorageFileType::FlagInfo).unwrap()
-        };
         for i in 0..8 {
             set_flag_has_local_override(&mut file, FlagValueType::Boolean, i, true).unwrap();
             let attribute =