From 436f1368cd53353d9f54a9f1495300340b0d46df Mon Sep 17 00:00:00 2001 From: Zhi Dou Date: Mon, 24 Jun 2024 21:14:15 +0000 Subject: [PATCH] flush mmap file to disk after updating The change was applied in the file in memory. Without explicitly calling msync, there is no guarantee that the changes are written back to disk. Thus the flag value may not be changed. Bug: b/348693143 Test: atest aconfig_storage_write_api.test.cpp aconfig_storage_write_api.test.rust Change-Id: I8b1e0cbbb90bcb3c2e5bbf418a6a1a9cd047c81b --- .../aconfig_storage_write_api.cpp | 34 +++++--- .../src/flag_info_update.rs | 8 +- .../src/flag_value_update.rs | 4 +- .../aconfig_storage_write_api/src/lib.rs | 22 +++-- .../tests/Android.bp | 6 ++ .../tests/storage_write_api_test.cpp | 84 ++++++++++++++++++- 6 files changed, 131 insertions(+), 27 deletions(-) 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 cabc65e40c..7b435746da 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,6 +1,7 @@ #include #include +#include #include #include @@ -13,8 +14,8 @@ namespace aconfig_storage { /// Map a storage file -android::base::Result map_mutable_storage_file( - std::string const& file) { +android::base::Result map_mutable_storage_file( + std::string const &file) { struct stat file_stat; if (stat(file.c_str(), &file_stat) < 0) { return android::base::ErrnoError() << "stat failed"; @@ -26,13 +27,13 @@ android::base::Result map_mutable_storage_file( size_t file_size = file_stat.st_size; - const int fd = open(file.c_str(), O_RDWR | O_NOFOLLOW | O_CLOEXEC); - if (fd == -1) { + android::base::unique_fd ufd(open(file.c_str(), O_RDWR | O_NOFOLLOW | O_CLOEXEC)); + if (ufd.get() == -1) { return android::base::ErrnoError() << "failed to open " << file; }; - void* const map_result = - mmap(nullptr, file_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); + void *const map_result = + mmap(nullptr, file_size, PROT_READ | PROT_WRITE, MAP_SHARED, ufd.get(), 0); if (map_result == MAP_FAILED) { return android::base::ErrnoError() << "mmap failed"; } @@ -46,47 +47,56 @@ android::base::Result map_mutable_storage_file( /// Set boolean flag value android::base::Result set_boolean_flag_value( - const MutableMappedStorageFile& file, + const MutableMappedStorageFile &file, uint32_t offset, bool value) { auto content = rust::Slice( - static_cast(file.file_ptr), file.file_size); + static_cast(file.file_ptr), file.file_size); auto update_cxx = update_boolean_flag_value_cxx(content, offset, value); if (!update_cxx.update_success) { return android::base::Error() << update_cxx.error_message.c_str(); } + if (!msync(static_cast(file.file_ptr) + update_cxx.offset, 1, MS_SYNC)) { + return android::base::ErrnoError() << "msync failed"; + } return {}; } /// Set if flag has server override android::base::Result set_flag_has_server_override( - const MutableMappedStorageFile& file, + const MutableMappedStorageFile &file, FlagValueType value_type, uint32_t offset, bool value) { auto content = rust::Slice( - static_cast(file.file_ptr), file.file_size); + static_cast(file.file_ptr), file.file_size); auto update_cxx = update_flag_has_server_override_cxx( content, static_cast(value_type), offset, value); if (!update_cxx.update_success) { return android::base::Error() << update_cxx.error_message.c_str(); } + if (!msync(static_cast(file.file_ptr) + update_cxx.offset, 1, MS_SYNC)) { + return android::base::ErrnoError() << "msync failed"; + } return {}; } /// Set if flag has local override android::base::Result set_flag_has_local_override( - const MutableMappedStorageFile& file, + const MutableMappedStorageFile &file, FlagValueType value_type, uint32_t offset, bool value) { auto content = rust::Slice( - static_cast(file.file_ptr), file.file_size); + static_cast(file.file_ptr), file.file_size); auto update_cxx = update_flag_has_local_override_cxx( content, static_cast(value_type), offset, value); if (!update_cxx.update_success) { return android::base::Error() << update_cxx.error_message.c_str(); } + if (!msync(static_cast(file.file_ptr) + update_cxx.offset, 1, MS_SYNC)) { + return android::base::ErrnoError() << "msync failed"; + } return {}; } diff --git a/tools/aconfig/aconfig_storage_write_api/src/flag_info_update.rs b/tools/aconfig/aconfig_storage_write_api/src/flag_info_update.rs index 6f03f128a5..7e6071340c 100644 --- a/tools/aconfig/aconfig_storage_write_api/src/flag_info_update.rs +++ b/tools/aconfig/aconfig_storage_write_api/src/flag_info_update.rs @@ -67,13 +67,13 @@ pub fn update_flag_has_server_override( flag_type: FlagValueType, flag_index: u32, value: bool, -) -> Result<(), AconfigStorageError> { +) -> Result { let (attribute, head) = get_flag_attribute_and_offset(buf, flag_type, flag_index)?; let has_override = (attribute & (FlagInfoBit::HasServerOverride as u8)) != 0; if has_override != value { buf[head] = (attribute ^ FlagInfoBit::HasServerOverride as u8).to_le_bytes()[0]; } - Ok(()) + Ok(head) } /// Set if flag has local override @@ -82,13 +82,13 @@ pub fn update_flag_has_local_override( flag_type: FlagValueType, flag_index: u32, value: bool, -) -> Result<(), AconfigStorageError> { +) -> Result { let (attribute, head) = get_flag_attribute_and_offset(buf, flag_type, flag_index)?; let has_override = (attribute & (FlagInfoBit::HasLocalOverride as u8)) != 0; if has_override != value { buf[head] = (attribute ^ FlagInfoBit::HasLocalOverride as u8).to_le_bytes()[0]; } - Ok(()) + Ok(head) } #[cfg(test)] diff --git a/tools/aconfig/aconfig_storage_write_api/src/flag_value_update.rs b/tools/aconfig/aconfig_storage_write_api/src/flag_value_update.rs index 0938715714..dd15c996a6 100644 --- a/tools/aconfig/aconfig_storage_write_api/src/flag_value_update.rs +++ b/tools/aconfig/aconfig_storage_write_api/src/flag_value_update.rs @@ -24,7 +24,7 @@ pub fn update_boolean_flag_value( buf: &mut [u8], flag_index: u32, flag_value: bool, -) -> Result<(), AconfigStorageError> { +) -> Result { let interpreted_header = FlagValueHeader::from_bytes(buf)?; if interpreted_header.version > FILE_VERSION { return Err(AconfigStorageError::HigherStorageFileVersion(anyhow!( @@ -43,7 +43,7 @@ pub fn update_boolean_flag_value( } buf[head] = u8::from(flag_value).to_le_bytes()[0]; - Ok(()) + Ok(head) } #[cfg(test)] diff --git a/tools/aconfig/aconfig_storage_write_api/src/lib.rs b/tools/aconfig/aconfig_storage_write_api/src/lib.rs index aec28def68..0396a63d4e 100644 --- a/tools/aconfig/aconfig_storage_write_api/src/lib.rs +++ b/tools/aconfig/aconfig_storage_write_api/src/lib.rs @@ -194,18 +194,21 @@ mod ffi { // Flag value update return for cc interlop pub struct BooleanFlagValueUpdateCXX { pub update_success: bool, + pub offset: usize, pub error_message: String, } // Flag has server override update return for cc interlop pub struct FlagHasServerOverrideUpdateCXX { pub update_success: bool, + pub offset: usize, pub error_message: String, } // Flag has local override update return for cc interlop pub struct FlagHasLocalOverrideUpdateCXX { pub update_success: bool, + pub offset: usize, pub error_message: String, } @@ -251,11 +254,14 @@ pub(crate) fn update_boolean_flag_value_cxx( value: bool, ) -> ffi::BooleanFlagValueUpdateCXX { match crate::flag_value_update::update_boolean_flag_value(file, offset, value) { - Ok(()) => { - ffi::BooleanFlagValueUpdateCXX { update_success: true, error_message: String::from("") } - } + Ok(head) => ffi::BooleanFlagValueUpdateCXX { + update_success: true, + offset: head, + error_message: String::from(""), + }, Err(errmsg) => ffi::BooleanFlagValueUpdateCXX { update_success: false, + offset: usize::MAX, error_message: format!("{:?}", errmsg), }, } @@ -272,18 +278,21 @@ pub(crate) fn update_flag_has_server_override_cxx( match crate::flag_info_update::update_flag_has_server_override( file, value_type, offset, value, ) { - Ok(()) => ffi::FlagHasServerOverrideUpdateCXX { + Ok(head) => ffi::FlagHasServerOverrideUpdateCXX { update_success: true, + offset: head, error_message: String::from(""), }, Err(errmsg) => ffi::FlagHasServerOverrideUpdateCXX { update_success: false, + offset: usize::MAX, error_message: format!("{:?}", errmsg), }, } } Err(errmsg) => ffi::FlagHasServerOverrideUpdateCXX { update_success: false, + offset: usize::MAX, error_message: format!("{:?}", errmsg), }, } @@ -300,18 +309,21 @@ pub(crate) fn update_flag_has_local_override_cxx( match crate::flag_info_update::update_flag_has_local_override( file, value_type, offset, value, ) { - Ok(()) => ffi::FlagHasLocalOverrideUpdateCXX { + Ok(head) => ffi::FlagHasLocalOverrideUpdateCXX { update_success: true, + offset: head, error_message: String::from(""), }, Err(errmsg) => ffi::FlagHasLocalOverrideUpdateCXX { update_success: false, + offset: usize::MAX, error_message: format!("{:?}", errmsg), }, } } Err(errmsg) => ffi::FlagHasLocalOverrideUpdateCXX { update_success: false, + offset: usize::MAX, error_message: format!("{:?}", errmsg), }, } diff --git a/tools/aconfig/aconfig_storage_write_api/tests/Android.bp b/tools/aconfig/aconfig_storage_write_api/tests/Android.bp index f6409b7090..d7afc1ac79 100644 --- a/tools/aconfig/aconfig_storage_write_api/tests/Android.bp +++ b/tools/aconfig/aconfig_storage_write_api/tests/Android.bp @@ -39,4 +39,10 @@ cc_test { "general-tests", ], ldflags: ["-Wl,--allow-multiple-definition"], + generated_headers: [ + "cxx-bridge-header", + "libcxx_aconfig_storage_read_api_bridge_header", + ], + generated_sources: ["libcxx_aconfig_storage_read_api_bridge_code"], + whole_static_libs: ["libaconfig_storage_read_api_cxx_bridge"], } 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 31183fa01b..133f5a0592 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 @@ -25,6 +25,9 @@ #include #include +#include "rust/cxx.h" +#include "aconfig_storage/lib.rs.h" + using namespace android::base; namespace api = aconfig_storage; @@ -85,6 +88,23 @@ TEST_F(AconfigStorageTest, test_boolean_flag_value_update) { ASSERT_TRUE(value.ok()); ASSERT_TRUE(*value); } + + // load the file on disk and check has been updated + std::ifstream file(flag_val, std::ios::binary | std::ios::ate); + std::streamsize size = file.tellg(); + file.seekg(0, std::ios::beg); + + std::vector buffer(size); + file.read(reinterpret_cast(buffer.data()), size); + + auto content = rust::Slice( + buffer.data(), mapped_file->file_size); + + for (int offset = 0; offset < 8; ++offset) { + auto value_cxx = get_boolean_flag_value_cxx(content, offset); + ASSERT_TRUE(value_cxx.query_success); + ASSERT_TRUE(value_cxx.flag_value); + } } /// Negative test to lock down the error when querying flag value out of range @@ -112,15 +132,43 @@ TEST_F(AconfigStorageTest, test_flag_has_server_override_update) { *mapped_file, api::FlagValueType::Boolean, offset); ASSERT_TRUE(attribute.ok()); ASSERT_TRUE(*attribute & api::FlagInfoBit::HasServerOverride); + } - update_result = api::set_flag_has_server_override( + // load the file on disk and check has been updated + std::ifstream file(flag_info, std::ios::binary | std::ios::ate); + std::streamsize size = file.tellg(); + file.seekg(0, std::ios::beg); + + std::vector buffer(size); + file.read(reinterpret_cast(buffer.data()), size); + + auto content = rust::Slice( + buffer.data(), mapped_file->file_size); + + for (int offset = 0; offset < 8; ++offset) { + auto attribute = get_flag_attribute_cxx(content, api::FlagValueType::Boolean, offset); + ASSERT_TRUE(attribute.query_success); + ASSERT_TRUE(attribute.flag_attribute & api::FlagInfoBit::HasServerOverride); + } + + for (int offset = 0; offset < 8; ++offset) { + auto update_result = api::set_flag_has_server_override( *mapped_file, api::FlagValueType::Boolean, offset, false); ASSERT_TRUE(update_result.ok()); - attribute = api::get_flag_attribute( + auto attribute = api::get_flag_attribute( *mapped_file, api::FlagValueType::Boolean, offset); ASSERT_TRUE(attribute.ok()); ASSERT_FALSE(*attribute & api::FlagInfoBit::HasServerOverride); } + + std::ifstream file2(flag_info, std::ios::binary); + buffer.clear(); + file2.read(reinterpret_cast(buffer.data()), size); + for (int offset = 0; offset < 8; ++offset) { + auto attribute = get_flag_attribute_cxx(content, api::FlagValueType::Boolean, offset); + ASSERT_TRUE(attribute.query_success); + ASSERT_FALSE(attribute.flag_attribute & api::FlagInfoBit::HasServerOverride); + } } /// Test to lock down storage flag has local override update api @@ -137,13 +185,41 @@ TEST_F(AconfigStorageTest, test_flag_has_local_override_update) { *mapped_file, api::FlagValueType::Boolean, offset); ASSERT_TRUE(attribute.ok()); ASSERT_TRUE(*attribute & api::FlagInfoBit::HasLocalOverride); + } - update_result = api::set_flag_has_local_override( + // load the file on disk and check has been updated + std::ifstream file(flag_info, std::ios::binary | std::ios::ate); + std::streamsize size = file.tellg(); + file.seekg(0, std::ios::beg); + + std::vector buffer(size); + file.read(reinterpret_cast(buffer.data()), size); + + auto content = rust::Slice( + buffer.data(), mapped_file->file_size); + + for (int offset = 0; offset < 8; ++offset) { + auto attribute = get_flag_attribute_cxx(content, api::FlagValueType::Boolean, offset); + ASSERT_TRUE(attribute.query_success); + ASSERT_TRUE(attribute.flag_attribute & api::FlagInfoBit::HasLocalOverride); + } + + for (int offset = 0; offset < 8; ++offset) { + auto update_result = api::set_flag_has_local_override( *mapped_file, api::FlagValueType::Boolean, offset, false); ASSERT_TRUE(update_result.ok()); - attribute = api::get_flag_attribute( + auto attribute = api::get_flag_attribute( *mapped_file, api::FlagValueType::Boolean, offset); ASSERT_TRUE(attribute.ok()); ASSERT_FALSE(*attribute & api::FlagInfoBit::HasLocalOverride); } + + std::ifstream file2(flag_info, std::ios::binary); + buffer.clear(); + file2.read(reinterpret_cast(buffer.data()), size); + for (int offset = 0; offset < 8; ++offset) { + auto attribute = get_flag_attribute_cxx(content, api::FlagValueType::Boolean, offset); + ASSERT_TRUE(attribute.query_success); + ASSERT_FALSE(attribute.flag_attribute & api::FlagInfoBit::HasLocalOverride); + } }