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