Merge "aconfig: update storage file mapping api" into main

8 files changed, 83 insertions, 60 deletions

diff --git a/tools/aconfig/aconfig/templates/cpp_source_file.template b/tools/aconfig/aconfig/templates/cpp_source_file.template
index 62664bc566..4c098c5b41 100644
--- a/tools/aconfig/aconfig/templates/cpp_source_file.template
+++ b/tools/aconfig/aconfig/templates/cpp_source_file.template
@@ -129,12 +129,14 @@ bool {header}_{item.flag_name}() \{
     }
 
     auto package_read_context = aconfig_storage::get_package_read_context(
-        *package_map_file, "{package}");
+        **package_map_file, "{package}");
     if (!package_read_context.ok()) \{
         ALOGI("error: failed to get package read context: %s", package_map_file.error().message().c_str());
         return result;
     }
 
+    delete *package_map_file;
+
     auto flag_val_map = aconfig_storage::get_mapped_file(
         "{item.container}",
         aconfig_storage::StorageFileType::flag_val);
@@ -144,13 +146,15 @@ bool {header}_{item.flag_name}() \{
     }
 
     auto value = aconfig_storage::get_boolean_flag_value(
-        *flag_val_map,
-        package_read_context->package_id + {item.flag_offset});
+        **flag_val_map,
+        package_read_context->boolean_start_index + {item.flag_offset});
     if (!value.ok()) \{
         ALOGI("error: failed to get flag val: %s", package_map_file.error().message().c_str());
         return result;
     }
 
+    delete *flag_val_map;
+
     if (*value != result) \{
         ALOGI("error: new storage value '%d' does not match current value '%d'", *value, result);
     } else \{
diff --git a/tools/aconfig/aconfig_storage_read_api/Android.bp b/tools/aconfig/aconfig_storage_read_api/Android.bp
index 217104b6b2..db362944c5 100644
--- a/tools/aconfig/aconfig_storage_read_api/Android.bp
+++ b/tools/aconfig/aconfig_storage_read_api/Android.bp
@@ -92,10 +92,10 @@ cc_library {
     static_libs: [
         "libaconfig_storage_protos_cc",
         "libprotobuf-cpp-lite",
-        "libbase",
     ],
     shared_libs: [
         "liblog",
+        "libbase",
     ],
     apex_available: [
         "//apex_available:platform",
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 8fe42ceddc..0aa936a9eb 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
@@ -20,6 +20,11 @@ namespace aconfig_storage {
 static constexpr char kAvailableStorageRecordsPb[] =
     "/metadata/aconfig/boot/available_storage_file_records.pb";
 
+/// destructor
+MappedStorageFile::~MappedStorageFile() {
+  munmap(file_ptr, file_size);
+}
+
 /// 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();
@@ -68,7 +73,7 @@ static Result<std::string> find_storage_file(
 namespace private_internal_api {
 
 /// Get mapped file implementation.
-Result<MappedStorageFile> get_mapped_file_impl(
+Result<MappedStorageFile*> get_mapped_file_impl(
     std::string const& pb_file,
     std::string const& container,
     StorageFileType file_type) {
@@ -82,7 +87,7 @@ Result<MappedStorageFile> get_mapped_file_impl(
 } // namespace private internal api
 
 /// Map a storage file
-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;
@@ -99,9 +104,9 @@ Result<MappedStorageFile> map_storage_file(std::string const& file) {
     return ErrnoError() << "mmap failed";
   }
 
-  auto mapped_file = MappedStorageFile();
-  mapped_file.file_ptr = map_result;
-  mapped_file.file_size = file_size;
+  auto mapped_file = new MappedStorageFile();
+  mapped_file->file_ptr = map_result;
+  mapped_file->file_size = file_size;
 
   return mapped_file;
 }
@@ -120,7 +125,7 @@ android::base::Result<FlagValueType> map_to_flag_value_type(
 }
 
 /// Get mapped storage file
-Result<MappedStorageFile> get_mapped_file(
+Result<MappedStorageFile*> get_mapped_file(
     std::string const& container,
     StorageFileType file_type) {
   return private_internal_api::get_mapped_file_impl(
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 2bd84fcf20..b8ee06a590 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
@@ -41,6 +41,7 @@ enum FlagInfoBit {
 struct MappedStorageFile {
   void* file_ptr;
   size_t file_size;
+  ~MappedStorageFile();
 };
 
 /// Package read context query result
@@ -60,7 +61,7 @@ struct FlagReadContext {
 /// DO NOT USE APIS IN THE FOLLOWING NAMESPACE DIRECTLY
 namespace private_internal_api {
 
-android::base::Result<MappedStorageFile> get_mapped_file_impl(
+android::base::Result<MappedStorageFile*> get_mapped_file_impl(
     std::string const& pb_file,
     std::string const& container,
     StorageFileType file_type);
@@ -68,7 +69,7 @@ android::base::Result<MappedStorageFile> get_mapped_file_impl(
 } // namespace private_internal_api
 
 /// Map a storage file
-android::base::Result<MappedStorageFile> map_storage_file(
+android::base::Result<MappedStorageFile*> map_storage_file(
     std::string const& file);
 
 
@@ -82,7 +83,7 @@ android::base::Result<FlagValueType> map_to_flag_value_type(
 /// \input container: stoarge container name
 /// \input file_type: storage file type enum
 /// \returns a MappedStorageFileQuery
-android::base::Result<MappedStorageFile> get_mapped_file(
+android::base::Result<MappedStorageFile*> get_mapped_file(
     std::string const& container,
     StorageFileType file_type);
 
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 cfd128d123..5393c49a7d 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
@@ -16,6 +16,7 @@
 
 #include <string>
 #include <vector>
+#include <memory>
 #include <cstdio>
 
 #include <sys/stat.h>
@@ -111,18 +112,19 @@ 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 = private_api::get_mapped_file_impl(
+  auto mapped_file_result = private_api::get_mapped_file_impl(
       storage_record_pb, "vendor", api::StorageFileType::package_map);
-  ASSERT_FALSE(mapped_file.ok());
-  ASSERT_EQ(mapped_file.error().message(),
+  ASSERT_FALSE(mapped_file_result.ok());
+  ASSERT_EQ(mapped_file_result.error().message(),
             "Unable to find storage files for container vendor");
 }
 
 /// Test to lock down storage package context query api
 TEST_F(AconfigStorageTest, test_package_context_query) {
-  auto mapped_file = private_api::get_mapped_file_impl(
+  auto mapped_file_result = private_api::get_mapped_file_impl(
       storage_record_pb, "mockup", api::StorageFileType::package_map);
-  ASSERT_TRUE(mapped_file.ok());
+  ASSERT_TRUE(mapped_file_result.ok());
+  auto mapped_file = std::unique_ptr<api::MappedStorageFile>(*mapped_file_result);
 
   auto context = api::get_package_read_context(
       *mapped_file, "com.android.aconfig.storage.test_1");
@@ -148,9 +150,10 @@ 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 = private_api::get_mapped_file_impl(
+  auto mapped_file_result = private_api::get_mapped_file_impl(
       storage_record_pb, "mockup", api::StorageFileType::package_map);
-  ASSERT_TRUE(mapped_file.ok());
+  ASSERT_TRUE(mapped_file_result.ok());
+  auto mapped_file = std::unique_ptr<api::MappedStorageFile>(*mapped_file_result);
 
   auto context = api::get_package_read_context(
       *mapped_file, "com.android.aconfig.storage.test_3");
@@ -160,9 +163,10 @@ 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 = private_api::get_mapped_file_impl(
+  auto mapped_file_result = private_api::get_mapped_file_impl(
       storage_record_pb, "mockup", api::StorageFileType::flag_map);
-  ASSERT_TRUE(mapped_file.ok());
+  ASSERT_TRUE(mapped_file_result.ok());
+  auto mapped_file = std::unique_ptr<api::MappedStorageFile>(*mapped_file_result);
 
   auto baseline = std::vector<std::tuple<int, std::string, api::StoredFlagType, int>>{
     {0, "enabled_ro", api::StoredFlagType::ReadOnlyBoolean, 1},
@@ -185,9 +189,10 @@ 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 = private_api::get_mapped_file_impl(
+  auto mapped_file_result = private_api::get_mapped_file_impl(
       storage_record_pb, "mockup", api::StorageFileType::flag_map);
-  ASSERT_TRUE(mapped_file.ok());
+  ASSERT_TRUE(mapped_file_result.ok());
+  auto mapped_file = std::unique_ptr<api::MappedStorageFile>(*mapped_file_result);
 
   auto context = api::get_flag_read_context(*mapped_file, 0, "none_exist");
   ASSERT_TRUE(context.ok());
@@ -200,9 +205,10 @@ 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 = private_api::get_mapped_file_impl(
+  auto mapped_file_result = private_api::get_mapped_file_impl(
       storage_record_pb, "mockup", api::StorageFileType::flag_val);
-  ASSERT_TRUE(mapped_file.ok());
+  ASSERT_TRUE(mapped_file_result.ok());
+  auto mapped_file = std::unique_ptr<api::MappedStorageFile>(*mapped_file_result);
 
   auto expected_value = std::vector<bool>{
     false, true, true, false, true, true, true, true};
@@ -215,9 +221,10 @@ 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 = private_api::get_mapped_file_impl(
+  auto mapped_file_result = private_api::get_mapped_file_impl(
       storage_record_pb, "mockup", api::StorageFileType::flag_val);
-  ASSERT_TRUE(mapped_file.ok());
+  ASSERT_TRUE(mapped_file_result.ok());
+  auto mapped_file = std::unique_ptr<api::MappedStorageFile>(*mapped_file_result);
 
   auto value = api::get_boolean_flag_value(*mapped_file, 8);
   ASSERT_FALSE(value.ok());
@@ -227,9 +234,10 @@ 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 = private_api::get_mapped_file_impl(
+  auto mapped_file_result = private_api::get_mapped_file_impl(
       storage_record_pb, "mockup", api::StorageFileType::flag_info);
-  ASSERT_TRUE(mapped_file.ok());
+  ASSERT_TRUE(mapped_file_result.ok());
+  auto mapped_file = std::unique_ptr<api::MappedStorageFile>(*mapped_file_result);
 
   auto expected_value = std::vector<bool>{
     true, false, true, true, false, false, false, true};
@@ -245,9 +253,10 @@ 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 = private_api::get_mapped_file_impl(
+  auto mapped_file_result = private_api::get_mapped_file_impl(
       storage_record_pb, "mockup", api::StorageFileType::flag_info);
-  ASSERT_TRUE(mapped_file.ok());
+  ASSERT_TRUE(mapped_file_result.ok());
+  auto mapped_file = std::unique_ptr<api::MappedStorageFile>(*mapped_file_result);
 
   auto attribute = api::get_flag_attribute(*mapped_file, api::FlagValueType::Boolean, 8);
   ASSERT_FALSE(attribute.ok());
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 cd57f4fb29..197486d246 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,8 +17,13 @@ using namespace android::base;
 
 namespace aconfig_storage {
 
+/// destructor
+MutableMappedStorageFile::~MutableMappedStorageFile() {
+  munmap(file_ptr, file_size);
+}
+
 /// Map a storage file
-Result<MutableMappedStorageFile> map_mutable_storage_file(std::string const& file) {
+Result<MutableMappedStorageFile*> map_mutable_storage_file(std::string const& file) {
   struct stat file_stat;
   if (stat(file.c_str(), &file_stat) < 0) {
     return ErrnoError() << "stat failed";
@@ -41,9 +46,9 @@ Result<MutableMappedStorageFile> map_mutable_storage_file(std::string const& fil
     return ErrnoError() << "mmap failed";
   }
 
-  auto mapped_file = MutableMappedStorageFile();
-  mapped_file.file_ptr = map_result;
-  mapped_file.file_size = file_size;
+  auto mapped_file = new MutableMappedStorageFile();
+  mapped_file->file_ptr = map_result;
+  mapped_file->file_size = file_size;
 
   return mapped_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 7148396b82..1eca1e0cda 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
@@ -14,10 +14,11 @@ namespace aconfig_storage {
 struct MutableMappedStorageFile{
   void* file_ptr;
   size_t file_size;
+  ~MutableMappedStorageFile();
 };
 
 /// Map a storage file
-Result<MutableMappedStorageFile> map_mutable_storage_file(
+Result<MutableMappedStorageFile*> map_mutable_storage_file(
     std::string const& file);
 
 /// Set boolean flag value
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 bd39e9e1de..aeede49306 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
@@ -78,14 +78,14 @@ TEST_F(AconfigStorageTest, test_non_writable_storage_file_mapping) {
 TEST_F(AconfigStorageTest, test_boolean_flag_value_update) {
   auto mapped_file_result = api::map_mutable_storage_file(flag_val);
   ASSERT_TRUE(mapped_file_result.ok());
+  auto mapped_file = std::unique_ptr<api::MutableMappedStorageFile>(*mapped_file_result);
 
-  auto mapped_file = *mapped_file_result;
+  auto ro_mapped_file = api::MappedStorageFile();
+  ro_mapped_file.file_ptr = mapped_file->file_ptr;
+  ro_mapped_file.file_size = mapped_file->file_size;
   for (int offset = 0; offset < 8; ++offset) {
-    auto update_result = api::set_boolean_flag_value(mapped_file, offset, true);
+    auto update_result = api::set_boolean_flag_value(*mapped_file, offset, true);
     ASSERT_TRUE(update_result.ok());
-    auto ro_mapped_file = api::MappedStorageFile();
-    ro_mapped_file.file_ptr = mapped_file.file_ptr;
-    ro_mapped_file.file_size = mapped_file.file_size;
     auto value = api::get_boolean_flag_value(ro_mapped_file, offset);
     ASSERT_TRUE(value.ok());
     ASSERT_TRUE(*value);
@@ -96,9 +96,9 @@ TEST_F(AconfigStorageTest, test_boolean_flag_value_update) {
 TEST_F(AconfigStorageTest, test_invalid_boolean_flag_value_update) {
   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 = std::unique_ptr<api::MutableMappedStorageFile>(*mapped_file_result);
 
-  auto update_result = api::set_boolean_flag_value(mapped_file, 8, true);
+  auto update_result = api::set_boolean_flag_value(*mapped_file, 8, true);
   ASSERT_FALSE(update_result.ok());
   ASSERT_EQ(update_result.error().message(),
             std::string("InvalidStorageFileOffset(Flag value offset goes beyond the end of the file.)"));
@@ -108,25 +108,24 @@ TEST_F(AconfigStorageTest, test_invalid_boolean_flag_value_update) {
 TEST_F(AconfigStorageTest, test_flag_has_server_override_update) {
   auto mapped_file_result = api::map_mutable_storage_file(flag_info);
   ASSERT_TRUE(mapped_file_result.ok());
-  auto mapped_file = *mapped_file_result;
+  auto mapped_file = std::unique_ptr<api::MutableMappedStorageFile>(*mapped_file_result);
+
+  auto ro_mapped_file = api::MappedStorageFile();
+  ro_mapped_file.file_ptr = mapped_file->file_ptr;
+  ro_mapped_file.file_size = mapped_file->file_size;
 
   for (int offset = 0; offset < 8; ++offset) {
     auto update_result = api::set_flag_has_server_override(
-        mapped_file, api::FlagValueType::Boolean, offset, true);
+        *mapped_file, api::FlagValueType::Boolean, offset, true);
     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;
     auto attribute = api::get_flag_attribute(
         ro_mapped_file, api::FlagValueType::Boolean, offset);
     ASSERT_TRUE(attribute.ok());
     ASSERT_TRUE(*attribute & api::FlagInfoBit::HasServerOverride);
 
     update_result = api::set_flag_has_server_override(
-        mapped_file, api::FlagValueType::Boolean, offset, false);
+        *mapped_file, api::FlagValueType::Boolean, offset, false);
     ASSERT_TRUE(update_result.ok());
-    ro_mapped_file.file_ptr = mapped_file.file_ptr;
-    ro_mapped_file.file_size = mapped_file.file_size;
     attribute = api::get_flag_attribute(
         ro_mapped_file, api::FlagValueType::Boolean, offset);
     ASSERT_TRUE(attribute.ok());
@@ -138,25 +137,24 @@ TEST_F(AconfigStorageTest, test_flag_has_server_override_update) {
 TEST_F(AconfigStorageTest, test_flag_has_local_override_update) {
   auto mapped_file_result = api::map_mutable_storage_file(flag_info);
   ASSERT_TRUE(mapped_file_result.ok());
-  auto mapped_file = *mapped_file_result;
+  auto mapped_file = std::unique_ptr<api::MutableMappedStorageFile>(*mapped_file_result);
+
+  auto ro_mapped_file = api::MappedStorageFile();
+  ro_mapped_file.file_ptr = mapped_file->file_ptr;
+  ro_mapped_file.file_size = mapped_file->file_size;
 
   for (int offset = 0; offset < 8; ++offset) {
     auto update_result = api::set_flag_has_local_override(
-        mapped_file, api::FlagValueType::Boolean, offset, true);
+        *mapped_file, api::FlagValueType::Boolean, offset, true);
     ASSERT_TRUE(update_result.ok());
-    auto ro_mapped_file = api::MappedStorageFile();
-    ro_mapped_file.file_ptr = mapped_file.file_ptr;
-    ro_mapped_file.file_size = mapped_file.file_size;
     auto attribute = api::get_flag_attribute(
         ro_mapped_file, api::FlagValueType::Boolean, offset);
     ASSERT_TRUE(attribute.ok());
     ASSERT_TRUE(*attribute & api::FlagInfoBit::HasLocalOverride);
 
     update_result = api::set_flag_has_local_override(
-        mapped_file, api::FlagValueType::Boolean, offset, false);
+        *mapped_file, api::FlagValueType::Boolean, offset, false);
     ASSERT_TRUE(update_result.ok());
-    ro_mapped_file.file_ptr = mapped_file.file_ptr;
-    ro_mapped_file.file_size = mapped_file.file_size;
     attribute = api::get_flag_attribute(
         ro_mapped_file, api::FlagValueType::Boolean, offset);
     ASSERT_TRUE(attribute.ok());