diff options
34 files changed, 560 insertions, 201 deletions
diff --git a/core/tasks/mke2fs-dist.mk b/core/tasks/mke2fs-dist.mk new file mode 100644 index 0000000000..3540c1f985 --- /dev/null +++ b/core/tasks/mke2fs-dist.mk @@ -0,0 +1,22 @@ +# Copyright (C) 2024 Google Inc. +# +# 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. + +# TODO: After Soong's recovery partition variation can be set to selectable +# and the meta_lic file duplication issue is resolved, move it to the +# dist section of the corresponding module's Android.bp. +my_dist_files := $(HOST_OUT_EXECUTABLES)/mke2fs +my_dist_files += $(HOST_OUT_EXECUTABLES)/make_f2fs +my_dist_files += $(HOST_OUT_EXECUTABLES)/make_f2fs_casefold +$(call dist-for-goals,dist_files sdk,$(my_dist_files)) +my_dist_files := diff --git a/tools/aconfig/aconfig/Android.bp b/tools/aconfig/aconfig/Android.bp index 5037783fb5..68521af91f 100644 --- a/tools/aconfig/aconfig/Android.bp +++ b/tools/aconfig/aconfig/Android.bp @@ -68,14 +68,6 @@ aconfig_values { ], } -aconfig_values { - name: "aconfig.test.flag.second_values", - package: "com.android.aconfig.test", - srcs: [ - "tests/third.values", - ], -} - aconfig_value_set { name: "aconfig.test.flag.value_set", values: [ diff --git a/tools/aconfig/aconfig/src/commands.rs b/tools/aconfig/aconfig/src/commands.rs index b5854165bc..797a893ff1 100644 --- a/tools/aconfig/aconfig/src/commands.rs +++ b/tools/aconfig/aconfig/src/commands.rs @@ -17,7 +17,7 @@ use anyhow::{bail, ensure, Context, Result}; use itertools::Itertools; use protobuf::Message; -use std::collections::HashMap; +use std::collections::{BTreeMap, HashMap}; use std::hash::Hasher; use std::io::Read; use std::path::PathBuf; @@ -422,30 +422,23 @@ where Ok(flag_ids) } -// Creates a fingerprint of the flag names. Sorts the vector. -pub fn compute_flags_fingerprint(flag_names: &mut Vec<String>) -> Result<u64> { - flag_names.sort(); - - let mut hasher = SipHasher13::new(); - for flag in flag_names { - hasher.write(flag.as_bytes()); - } - Ok(hasher.finish()) -} - #[allow(dead_code)] // TODO: b/316357686 - Use fingerprint in codegen to // protect hardcoded offset reads. -fn compute_fingerprint_from_parsed_flags(flags: ProtoParsedFlags) -> Result<u64> { - let separated_flags: Vec<ProtoParsedFlag> = flags.parsed_flag.into_iter().collect::<Vec<_>>(); +pub fn compute_flag_offsets_fingerprint(flags_map: &HashMap<String, u16>) -> Result<u64> { + let mut hasher = SipHasher13::new(); - // All flags must belong to the same package as the fingerprint is per-package. - let Some(_package) = find_unique_package(&separated_flags) else { - bail!("No parsed flags, or the parsed flags use different packages."); - }; + // Need to sort to ensure the data is added to the hasher in the same order + // each run. + let sorted_map: BTreeMap<&String, &u16> = flags_map.iter().collect(); - let mut flag_names = - separated_flags.into_iter().map(|flag| flag.name.unwrap()).collect::<Vec<_>>(); - compute_flags_fingerprint(&mut flag_names) + for (flag, offset) in sorted_map { + // See https://docs.rs/siphasher/latest/siphasher/#note for use of write + // over write_i16. Similarly, use to_be_bytes rather than to_ne_bytes to + // ensure consistency. + hasher.write(flag.as_bytes()); + hasher.write(&offset.to_be_bytes()); + } + Ok(hasher.finish()) } #[cfg(test)] @@ -456,47 +449,16 @@ mod tests { #[test] fn test_offset_fingerprint() { let parsed_flags = crate::test::parse_test_flags(); - let expected_fingerprint: u64 = 5801144784618221668; + let package = find_unique_package(&parsed_flags.parsed_flag).unwrap().to_string(); + let flag_ids = assign_flag_ids(&package, parsed_flags.parsed_flag.iter()).unwrap(); + let expected_fingerprint = 10709892481002252132u64; - let hash_result = compute_fingerprint_from_parsed_flags(parsed_flags); + let hash_result = compute_flag_offsets_fingerprint(&flag_ids); assert_eq!(hash_result.unwrap(), expected_fingerprint); } #[test] - fn test_offset_fingerprint_matches_from_package() { - let parsed_flags: ProtoParsedFlags = crate::test::parse_test_flags(); - - // All test flags are in the same package, so fingerprint from all of them. - let result_from_parsed_flags = compute_fingerprint_from_parsed_flags(parsed_flags.clone()); - - let mut flag_names_vec = parsed_flags - .parsed_flag - .clone() - .into_iter() - .map(|flag| flag.name.unwrap()) - .map(String::from) - .collect::<Vec<_>>(); - let result_from_names = compute_flags_fingerprint(&mut flag_names_vec); - - // Assert the same hash is generated for each case. - assert_eq!(result_from_parsed_flags.unwrap(), result_from_names.unwrap()); - } - - #[test] - fn test_offset_fingerprint_different_packages_does_not_match() { - // Parse flags from two packages. - let parsed_flags: ProtoParsedFlags = crate::test::parse_test_flags(); - let second_parsed_flags = crate::test::parse_second_package_flags(); - - let result_from_parsed_flags = compute_fingerprint_from_parsed_flags(parsed_flags).unwrap(); - let second_result = compute_fingerprint_from_parsed_flags(second_parsed_flags).unwrap(); - - // Different flags should have a different fingerprint. - assert_ne!(result_from_parsed_flags, second_result); - } - - #[test] fn test_parse_flags() { let parsed_flags = crate::test::parse_test_flags(); // calls parse_flags aconfig_protos::parsed_flags::verify_fields(&parsed_flags).unwrap(); diff --git a/tools/aconfig/aconfig/src/storage/mod.rs b/tools/aconfig/aconfig/src/storage/mod.rs index efce24e422..73339f24b3 100644 --- a/tools/aconfig/aconfig/src/storage/mod.rs +++ b/tools/aconfig/aconfig/src/storage/mod.rs @@ -25,14 +25,12 @@ use crate::storage::{ flag_table::create_flag_table, flag_value::create_flag_value, package_table::create_package_table, }; -use aconfig_protos::ProtoParsedFlag; -use aconfig_protos::ProtoParsedFlags; +use aconfig_protos::{ProtoParsedFlag, ProtoParsedFlags}; use aconfig_storage_file::StorageFileType; pub struct FlagPackage<'a> { pub package_name: &'a str, pub package_id: u32, - pub fingerprint: u64, pub flag_names: HashSet<&'a str>, pub boolean_flags: Vec<&'a ProtoParsedFlag>, // The index of the first boolean flag in this aconfig package among all boolean @@ -45,7 +43,6 @@ impl<'a> FlagPackage<'a> { FlagPackage { package_name, package_id, - fingerprint: 0, flag_names: HashSet::new(), boolean_flags: vec![], boolean_start_index: 0, @@ -81,8 +78,6 @@ where for p in packages.iter_mut() { p.boolean_start_index = boolean_start_index; boolean_start_index += p.boolean_flags.len() as u32; - - // TODO: b/316357686 - Calculate fingerprint and add to package. } packages @@ -120,8 +115,6 @@ mod tests { use super::*; use crate::Input; - use aconfig_protos::ProtoParsedFlags; - pub fn parse_all_test_flags() -> Vec<ProtoParsedFlags> { let aconfig_files = [ ( diff --git a/tools/aconfig/aconfig/src/storage/package_table.rs b/tools/aconfig/aconfig/src/storage/package_table.rs index 33bb0774e0..c53602f9cb 100644 --- a/tools/aconfig/aconfig/src/storage/package_table.rs +++ b/tools/aconfig/aconfig/src/storage/package_table.rs @@ -48,7 +48,6 @@ impl PackageTableNodeWrapper { let node = PackageTableNode { package_name: String::from(package.package_name), package_id: package.package_id, - fingerprint: package.fingerprint, boolean_start_index: package.boolean_start_index, next_offset: None, }; diff --git a/tools/aconfig/aconfig/src/test.rs b/tools/aconfig/aconfig/src/test.rs index a19b372aac..7409cda6e8 100644 --- a/tools/aconfig/aconfig/src/test.rs +++ b/tools/aconfig/aconfig/src/test.rs @@ -295,24 +295,6 @@ parsed_flag { aconfig_protos::parsed_flags::try_from_binary_proto(&bytes).unwrap() } - pub fn parse_second_package_flags() -> ProtoParsedFlags { - let bytes = crate::commands::parse_flags( - "com.android.aconfig.second_test", - Some("system"), - vec![Input { - source: "tests/test_second_package.aconfig".to_string(), - reader: Box::new(include_bytes!("../tests/test_second_package.aconfig").as_slice()), - }], - vec![Input { - source: "tests/third.values".to_string(), - reader: Box::new(include_bytes!("../tests/third.values").as_slice()), - }], - crate::commands::DEFAULT_FLAG_PERMISSION, - ) - .unwrap(); - aconfig_protos::parsed_flags::try_from_binary_proto(&bytes).unwrap() - } - pub fn first_significant_code_diff(a: &str, b: &str) -> Option<String> { let a = a.lines().map(|line| line.trim_start()).filter(|line| !line.is_empty()); let b = b.lines().map(|line| line.trim_start()).filter(|line| !line.is_empty()); diff --git a/tools/aconfig/aconfig/tests/test.aconfig b/tools/aconfig/aconfig/tests/test.aconfig index a818b2332e..c11508aabc 100644 --- a/tools/aconfig/aconfig/tests/test.aconfig +++ b/tools/aconfig/aconfig/tests/test.aconfig @@ -86,4 +86,4 @@ flag { bug: "111" is_fixed_read_only: true is_exported: true -} +}
\ No newline at end of file diff --git a/tools/aconfig/aconfig/tests/test_second_package.aconfig b/tools/aconfig/aconfig/tests/test_second_package.aconfig deleted file mode 100644 index a8740b85dd..0000000000 --- a/tools/aconfig/aconfig/tests/test_second_package.aconfig +++ /dev/null @@ -1,12 +0,0 @@ -package: "com.android.aconfig.second_test" -container: "system" - -flag { - name: "testing_flag" - namespace: "another_namespace" - description: "This is a flag for testing." - bug: "123" - metadata { - purpose: PURPOSE_UNSPECIFIED - } -} diff --git a/tools/aconfig/aconfig/tests/third.values b/tools/aconfig/aconfig/tests/third.values deleted file mode 100644 index 675832a4bc..0000000000 --- a/tools/aconfig/aconfig/tests/third.values +++ /dev/null @@ -1,6 +0,0 @@ -flag_value { - package: "com.android.aconfig.second_test" - name: "testing_flag" - state: DISABLED - permission: READ_WRITE -} diff --git a/tools/aconfig/aconfig_storage_file/src/flag_info.rs b/tools/aconfig/aconfig_storage_file/src/flag_info.rs index a49756d012..f090396901 100644 --- a/tools/aconfig/aconfig_storage_file/src/flag_info.rs +++ b/tools/aconfig/aconfig_storage_file/src/flag_info.rs @@ -227,7 +227,7 @@ mod tests { let bytes = &flag_info_list.into_bytes(); let mut head = 0; let version = read_u32_from_bytes(bytes, &mut head).unwrap(); - assert_eq!(version, 2); + assert_eq!(version, 1); } #[test] diff --git a/tools/aconfig/aconfig_storage_file/src/flag_table.rs b/tools/aconfig/aconfig_storage_file/src/flag_table.rs index be82c63028..0588fe5039 100644 --- a/tools/aconfig/aconfig_storage_file/src/flag_table.rs +++ b/tools/aconfig/aconfig_storage_file/src/flag_table.rs @@ -253,7 +253,7 @@ mod tests { let bytes = &flag_table.into_bytes(); let mut head = 0; let version = read_u32_from_bytes(bytes, &mut head).unwrap(); - assert_eq!(version, 2); + assert_eq!(version, 1); } #[test] diff --git a/tools/aconfig/aconfig_storage_file/src/flag_value.rs b/tools/aconfig/aconfig_storage_file/src/flag_value.rs index c4cf29451e..b64c10ecdd 100644 --- a/tools/aconfig/aconfig_storage_file/src/flag_value.rs +++ b/tools/aconfig/aconfig_storage_file/src/flag_value.rs @@ -159,7 +159,7 @@ mod tests { let bytes = &flag_value_list.into_bytes(); let mut head = 0; let version = read_u32_from_bytes(bytes, &mut head).unwrap(); - assert_eq!(version, 2); + assert_eq!(version, 1); } #[test] diff --git a/tools/aconfig/aconfig_storage_file/src/lib.rs b/tools/aconfig/aconfig_storage_file/src/lib.rs index 19d0e51e61..cf52bc017d 100644 --- a/tools/aconfig/aconfig_storage_file/src/lib.rs +++ b/tools/aconfig/aconfig_storage_file/src/lib.rs @@ -58,7 +58,7 @@ use crate::AconfigStorageError::{ }; /// Storage file version -pub const FILE_VERSION: u32 = 2; +pub const FILE_VERSION: u32 = 1; /// Good hash table prime number pub(crate) const HASH_PRIMES: [u32; 29] = [ @@ -254,16 +254,6 @@ pub fn read_u32_from_bytes(buf: &[u8], head: &mut usize) -> Result<u32, AconfigS Ok(val) } -// Read and parse bytes as u64 -pub fn read_u64_from_bytes(buf: &[u8], head: &mut usize) -> Result<u64, AconfigStorageError> { - let val = - u64::from_le_bytes(buf[*head..*head + 8].try_into().map_err(|errmsg| { - BytesParseFail(anyhow!("fail to parse u64 from bytes: {}", errmsg)) - })?); - *head += 8; - Ok(val) -} - /// Read and parse bytes as string pub(crate) fn read_str_from_bytes( buf: &[u8], diff --git a/tools/aconfig/aconfig_storage_file/src/package_table.rs b/tools/aconfig/aconfig_storage_file/src/package_table.rs index 350f072df2..a5bd9e6446 100644 --- a/tools/aconfig/aconfig_storage_file/src/package_table.rs +++ b/tools/aconfig/aconfig_storage_file/src/package_table.rs @@ -17,10 +17,7 @@ //! package table module defines the package table file format and methods for serialization //! and deserialization -use crate::{ - get_bucket_index, read_str_from_bytes, read_u32_from_bytes, read_u64_from_bytes, - read_u8_from_bytes, -}; +use crate::{get_bucket_index, read_str_from_bytes, read_u32_from_bytes, read_u8_from_bytes}; use crate::{AconfigStorageError, StorageFileType}; use anyhow::anyhow; use serde::{Deserialize, Serialize}; @@ -100,7 +97,6 @@ impl PackageTableHeader { pub struct PackageTableNode { pub package_name: String, pub package_id: u32, - pub fingerprint: u64, // The index of the first boolean flag in this aconfig package among all boolean // flags in this container. pub boolean_start_index: u32, @@ -112,12 +108,8 @@ impl fmt::Debug for PackageTableNode { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { writeln!( f, - "Package: {}, Id: {}, Fingerprint: {}, Boolean flag start index: {}, Next: {:?}", - self.package_name, - self.package_id, - self.fingerprint, - self.boolean_start_index, - self.next_offset + "Package: {}, Id: {}, Boolean flag start index: {}, Next: {:?}", + self.package_name, self.package_id, self.boolean_start_index, self.next_offset )?; Ok(()) } @@ -131,7 +123,6 @@ impl PackageTableNode { result.extend_from_slice(&(name_bytes.len() as u32).to_le_bytes()); result.extend_from_slice(name_bytes); result.extend_from_slice(&self.package_id.to_le_bytes()); - result.extend_from_slice(&self.fingerprint.to_le_bytes()); result.extend_from_slice(&self.boolean_start_index.to_le_bytes()); result.extend_from_slice(&self.next_offset.unwrap_or(0).to_le_bytes()); result @@ -143,7 +134,6 @@ impl PackageTableNode { let node = Self { package_name: read_str_from_bytes(bytes, &mut head)?, package_id: read_u32_from_bytes(bytes, &mut head)?, - fingerprint: read_u64_from_bytes(bytes, &mut head)?, boolean_start_index: read_u32_from_bytes(bytes, &mut head)?, next_offset: match read_u32_from_bytes(bytes, &mut head)? { 0 => None, @@ -261,7 +251,7 @@ mod tests { let bytes = &package_table.into_bytes(); let mut head = 0; let version = read_u32_from_bytes(bytes, &mut head).unwrap(); - assert_eq!(version, 2); + assert_eq!(version, 1); } #[test] diff --git a/tools/aconfig/aconfig_storage_file/src/test_utils.rs b/tools/aconfig/aconfig_storage_file/src/test_utils.rs index 11e2dc688e..106666c47f 100644 --- a/tools/aconfig/aconfig_storage_file/src/test_utils.rs +++ b/tools/aconfig/aconfig_storage_file/src/test_utils.rs @@ -26,33 +26,30 @@ use tempfile::NamedTempFile; pub fn create_test_package_table() -> PackageTable { let header = PackageTableHeader { - version: 2, + version: 1, container: String::from("mockup"), file_type: StorageFileType::PackageMap as u8, - file_size: 233, + file_size: 209, num_packages: 3, bucket_offset: 31, node_offset: 59, }; - let buckets: Vec<Option<u32>> = vec![Some(59), None, None, Some(117), None, None, None]; + let buckets: Vec<Option<u32>> = vec![Some(59), None, None, Some(109), None, None, None]; let first_node = PackageTableNode { package_name: String::from("com.android.aconfig.storage.test_2"), package_id: 1, - fingerprint: 0, boolean_start_index: 3, next_offset: None, }; let second_node = PackageTableNode { package_name: String::from("com.android.aconfig.storage.test_1"), package_id: 0, - fingerprint: 0, boolean_start_index: 0, - next_offset: Some(175), + next_offset: Some(159), }; let third_node = PackageTableNode { package_name: String::from("com.android.aconfig.storage.test_4"), package_id: 2, - fingerprint: 0, boolean_start_index: 6, next_offset: None, }; @@ -81,7 +78,7 @@ impl FlagTableNode { pub fn create_test_flag_table() -> FlagTable { let header = FlagTableHeader { - version: 2, + version: 1, container: String::from("mockup"), file_type: StorageFileType::FlagMap as u8, file_size: 321, @@ -123,7 +120,7 @@ pub fn create_test_flag_table() -> FlagTable { pub fn create_test_flag_value_list() -> FlagValueList { let header = FlagValueHeader { - version: 2, + version: 1, container: String::from("mockup"), file_type: StorageFileType::FlagVal as u8, file_size: 35, @@ -136,7 +133,7 @@ pub fn create_test_flag_value_list() -> FlagValueList { pub fn create_test_flag_info_list() -> FlagInfoList { let header = FlagInfoHeader { - version: 2, + version: 1, container: String::from("mockup"), file_type: StorageFileType::FlagInfo as u8, file_size: 35, diff --git a/tools/aconfig/aconfig_storage_file/srcs/android/aconfig/storage/ByteBufferReader.java b/tools/aconfig/aconfig_storage_file/srcs/android/aconfig/storage/ByteBufferReader.java index 44a82ee1c7..4bea0836f0 100644 --- a/tools/aconfig/aconfig_storage_file/srcs/android/aconfig/storage/ByteBufferReader.java +++ b/tools/aconfig/aconfig_storage_file/srcs/android/aconfig/storage/ByteBufferReader.java @@ -37,10 +37,6 @@ public class ByteBufferReader { return Short.toUnsignedInt(mByteBuffer.getShort()); } - public long readLong() { - return mByteBuffer.getLong(); - } - public int readInt() { return this.mByteBuffer.getInt(); } diff --git a/tools/aconfig/aconfig_storage_file/srcs/android/aconfig/storage/PackageTable.java b/tools/aconfig/aconfig_storage_file/srcs/android/aconfig/storage/PackageTable.java index f1288f5ea6..773b882f4a 100644 --- a/tools/aconfig/aconfig_storage_file/srcs/android/aconfig/storage/PackageTable.java +++ b/tools/aconfig/aconfig_storage_file/srcs/android/aconfig/storage/PackageTable.java @@ -118,7 +118,6 @@ public class PackageTable { private String mPackageName; private int mPackageId; - private long mFingerprint; private int mBooleanStartIndex; private int mNextOffset; @@ -126,7 +125,6 @@ public class PackageTable { Node node = new Node(); node.mPackageName = reader.readString(); node.mPackageId = reader.readInt(); - node.mFingerprint = reader.readLong(); node.mBooleanStartIndex = reader.readInt(); node.mNextOffset = reader.readInt(); node.mNextOffset = node.mNextOffset == 0 ? -1 : node.mNextOffset; @@ -152,7 +150,6 @@ public class PackageTable { return Objects.equals(mPackageName, other.mPackageName) && mPackageId == other.mPackageId && mBooleanStartIndex == other.mBooleanStartIndex - && mFingerprint == other.mFingerprint && mNextOffset == other.mNextOffset; } @@ -168,10 +165,6 @@ public class PackageTable { return mBooleanStartIndex; } - public long getFingerprint() { - return mFingerprint; - } - public int getNextOffset() { return mNextOffset; } diff --git a/tools/aconfig/aconfig_storage_file/tests/flag.info b/tools/aconfig/aconfig_storage_file/tests/flag.info Binary files differindex 9db7fde7ae..6223edf369 100644 --- a/tools/aconfig/aconfig_storage_file/tests/flag.info +++ b/tools/aconfig/aconfig_storage_file/tests/flag.info diff --git a/tools/aconfig/aconfig_storage_file/tests/flag.map b/tools/aconfig/aconfig_storage_file/tests/flag.map Binary files differindex cf4685ceb4..e868f53d7e 100644 --- a/tools/aconfig/aconfig_storage_file/tests/flag.map +++ b/tools/aconfig/aconfig_storage_file/tests/flag.map diff --git a/tools/aconfig/aconfig_storage_file/tests/flag.val b/tools/aconfig/aconfig_storage_file/tests/flag.val Binary files differindex 37d4750206..ed203d4d13 100644 --- a/tools/aconfig/aconfig_storage_file/tests/flag.val +++ b/tools/aconfig/aconfig_storage_file/tests/flag.val diff --git a/tools/aconfig/aconfig_storage_file/tests/package.map b/tools/aconfig/aconfig_storage_file/tests/package.map Binary files differindex 358010cf37..6c46a0339c 100644 --- a/tools/aconfig/aconfig_storage_file/tests/package.map +++ b/tools/aconfig/aconfig_storage_file/tests/package.map diff --git a/tools/aconfig/aconfig_storage_file/tests/srcs/FlagTableTest.java b/tools/aconfig/aconfig_storage_file/tests/srcs/FlagTableTest.java index e3b02cd666..fd40d4c4ef 100644 --- a/tools/aconfig/aconfig_storage_file/tests/srcs/FlagTableTest.java +++ b/tools/aconfig/aconfig_storage_file/tests/srcs/FlagTableTest.java @@ -33,7 +33,7 @@ public class FlagTableTest { public void testFlagTable_rightHeader() throws Exception { FlagTable flagTable = FlagTable.fromBytes(TestDataUtils.getTestFlagMapByteBuffer()); FlagTable.Header header = flagTable.getHeader(); - assertEquals(2, header.getVersion()); + assertEquals(1, header.getVersion()); assertEquals("mockup", header.getContainer()); assertEquals(FileType.FLAG_MAP, header.getFileType()); assertEquals(321, header.getFileSize()); diff --git a/tools/aconfig/aconfig_storage_file/tests/srcs/FlagValueListTest.java b/tools/aconfig/aconfig_storage_file/tests/srcs/FlagValueListTest.java index ebc231c2dd..1b0de630c7 100644 --- a/tools/aconfig/aconfig_storage_file/tests/srcs/FlagValueListTest.java +++ b/tools/aconfig/aconfig_storage_file/tests/srcs/FlagValueListTest.java @@ -36,7 +36,7 @@ public class FlagValueListTest { FlagValueList flagValueList = FlagValueList.fromBytes(TestDataUtils.getTestFlagValByteBuffer()); FlagValueList.Header header = flagValueList.getHeader(); - assertEquals(2, header.getVersion()); + assertEquals(1, header.getVersion()); assertEquals("mockup", header.getContainer()); assertEquals(FileType.FLAG_VAL, header.getFileType()); assertEquals(35, header.getFileSize()); diff --git a/tools/aconfig/aconfig_storage_file/tests/srcs/PackageTableTest.java b/tools/aconfig/aconfig_storage_file/tests/srcs/PackageTableTest.java index 6d56ceed96..e7e19d8d51 100644 --- a/tools/aconfig/aconfig_storage_file/tests/srcs/PackageTableTest.java +++ b/tools/aconfig/aconfig_storage_file/tests/srcs/PackageTableTest.java @@ -20,6 +20,7 @@ import static org.junit.Assert.assertEquals; import android.aconfig.storage.FileType; import android.aconfig.storage.PackageTable; + import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -27,40 +28,42 @@ import org.junit.runners.JUnit4; @RunWith(JUnit4.class) public class PackageTableTest { - @Test - public void testPackageTable_rightHeader() throws Exception { - PackageTable packageTable = PackageTable.fromBytes(TestDataUtils.getTestPackageMapByteBuffer()); - PackageTable.Header header = packageTable.getHeader(); - assertEquals(2, header.getVersion()); - assertEquals("mockup", header.getContainer()); - assertEquals(FileType.PACKAGE_MAP, header.getFileType()); - assertEquals(209, header.getFileSize()); - assertEquals(3, header.getNumPackages()); - assertEquals(31, header.getBucketOffset()); - assertEquals(59, header.getNodeOffset()); - } + @Test + public void testPackageTable_rightHeader() throws Exception { + PackageTable packageTable = + PackageTable.fromBytes(TestDataUtils.getTestPackageMapByteBuffer()); + PackageTable.Header header = packageTable.getHeader(); + assertEquals(1, header.getVersion()); + assertEquals("mockup", header.getContainer()); + assertEquals(FileType.PACKAGE_MAP, header.getFileType()); + assertEquals(209, header.getFileSize()); + assertEquals(3, header.getNumPackages()); + assertEquals(31, header.getBucketOffset()); + assertEquals(59, header.getNodeOffset()); + } - @Test - public void testPackageTable_rightNode() throws Exception { - PackageTable packageTable = PackageTable.fromBytes(TestDataUtils.getTestPackageMapByteBuffer()); + @Test + public void testPackageTable_rightNode() throws Exception { + PackageTable packageTable = + PackageTable.fromBytes(TestDataUtils.getTestPackageMapByteBuffer()); - PackageTable.Node node1 = packageTable.get("com.android.aconfig.storage.test_1"); - PackageTable.Node node2 = packageTable.get("com.android.aconfig.storage.test_2"); - PackageTable.Node node4 = packageTable.get("com.android.aconfig.storage.test_4"); + PackageTable.Node node1 = packageTable.get("com.android.aconfig.storage.test_1"); + PackageTable.Node node2 = packageTable.get("com.android.aconfig.storage.test_2"); + PackageTable.Node node4 = packageTable.get("com.android.aconfig.storage.test_4"); - assertEquals("com.android.aconfig.storage.test_1", node1.getPackageName()); - assertEquals("com.android.aconfig.storage.test_2", node2.getPackageName()); - assertEquals("com.android.aconfig.storage.test_4", node4.getPackageName()); + assertEquals("com.android.aconfig.storage.test_1", node1.getPackageName()); + assertEquals("com.android.aconfig.storage.test_2", node2.getPackageName()); + assertEquals("com.android.aconfig.storage.test_4", node4.getPackageName()); - assertEquals(0, node1.getPackageId()); - assertEquals(1, node2.getPackageId()); - assertEquals(2, node4.getPackageId()); + assertEquals(0, node1.getPackageId()); + assertEquals(1, node2.getPackageId()); + assertEquals(2, node4.getPackageId()); - assertEquals(0, node1.getBooleanStartIndex()); - assertEquals(3, node2.getBooleanStartIndex()); - assertEquals(6, node4.getBooleanStartIndex()); + assertEquals(0, node1.getBooleanStartIndex()); + assertEquals(3, node2.getBooleanStartIndex()); + assertEquals(6, node4.getBooleanStartIndex()); - assertEquals(175, node1.getNextOffset()); + assertEquals(159, node1.getNextOffset()); assertEquals(-1, node2.getNextOffset()); assertEquals(-1, node4.getNextOffset()); } diff --git a/tools/aconfig/aconfig_storage_read_api/src/lib.rs b/tools/aconfig/aconfig_storage_read_api/src/lib.rs index 59aa749c74..d76cf3fe4e 100644 --- a/tools/aconfig/aconfig_storage_read_api/src/lib.rs +++ b/tools/aconfig/aconfig_storage_read_api/src/lib.rs @@ -507,9 +507,9 @@ mod tests { #[test] // this test point locks down flag storage file version number query api fn test_storage_version_query() { - assert_eq!(get_storage_file_version("./tests/package.map").unwrap(), 2); - assert_eq!(get_storage_file_version("./tests/flag.map").unwrap(), 2); - assert_eq!(get_storage_file_version("./tests/flag.val").unwrap(), 2); - assert_eq!(get_storage_file_version("./tests/flag.info").unwrap(), 2); + assert_eq!(get_storage_file_version("./tests/package.map").unwrap(), 1); + assert_eq!(get_storage_file_version("./tests/flag.map").unwrap(), 1); + assert_eq!(get_storage_file_version("./tests/flag.val").unwrap(), 1); + assert_eq!(get_storage_file_version("./tests/flag.info").unwrap(), 1); } } diff --git a/tools/aconfig/aconfig_storage_read_api/tests/flag.info b/tools/aconfig/aconfig_storage_read_api/tests/flag.info Binary files differindex 9db7fde7ae..6223edf369 100644 --- a/tools/aconfig/aconfig_storage_read_api/tests/flag.info +++ b/tools/aconfig/aconfig_storage_read_api/tests/flag.info diff --git a/tools/aconfig/aconfig_storage_read_api/tests/flag.map b/tools/aconfig/aconfig_storage_read_api/tests/flag.map Binary files differindex cf4685ceb4..e868f53d7e 100644 --- a/tools/aconfig/aconfig_storage_read_api/tests/flag.map +++ b/tools/aconfig/aconfig_storage_read_api/tests/flag.map diff --git a/tools/aconfig/aconfig_storage_read_api/tests/flag.val b/tools/aconfig/aconfig_storage_read_api/tests/flag.val Binary files differindex 37d4750206..ed203d4d13 100644 --- a/tools/aconfig/aconfig_storage_read_api/tests/flag.val +++ b/tools/aconfig/aconfig_storage_read_api/tests/flag.val diff --git a/tools/aconfig/aconfig_storage_read_api/tests/package.map b/tools/aconfig/aconfig_storage_read_api/tests/package.map Binary files differindex 358010cf37..6c46a0339c 100644 --- a/tools/aconfig/aconfig_storage_read_api/tests/package.map +++ b/tools/aconfig/aconfig_storage_read_api/tests/package.map 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 58460d1ac7..6d29045efe 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 @@ -80,16 +80,16 @@ class AconfigStorageTest : public ::testing::Test { TEST_F(AconfigStorageTest, test_storage_version_query) { auto version = api::get_storage_file_version(package_map); ASSERT_TRUE(version.ok()); - ASSERT_EQ(*version, 2); + ASSERT_EQ(*version, 1); version = api::get_storage_file_version(flag_map); ASSERT_TRUE(version.ok()); - ASSERT_EQ(*version, 2); + ASSERT_EQ(*version, 1); version = api::get_storage_file_version(flag_val); ASSERT_TRUE(version.ok()); - ASSERT_EQ(*version, 2); + ASSERT_EQ(*version, 1); version = api::get_storage_file_version(flag_info); ASSERT_TRUE(version.ok()); - ASSERT_EQ(*version, 2); + ASSERT_EQ(*version, 1); } /// Negative test to lock down the error when mapping none exist storage files 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 bd1b5843f1..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 @@ -200,9 +200,9 @@ mod aconfig_storage_rust_test { #[test] fn test_storage_version_query() { - assert_eq!(get_storage_file_version("./package.map").unwrap(), 2); - assert_eq!(get_storage_file_version("./flag.map").unwrap(), 2); - assert_eq!(get_storage_file_version("./flag.val").unwrap(), 2); - assert_eq!(get_storage_file_version("./flag.info").unwrap(), 2); + assert_eq!(get_storage_file_version("./package.map").unwrap(), 1); + assert_eq!(get_storage_file_version("./flag.map").unwrap(), 1); + assert_eq!(get_storage_file_version("./flag.val").unwrap(), 1); + assert_eq!(get_storage_file_version("./flag.info").unwrap(), 1); } } diff --git a/tools/edit_monitor/Android.bp b/tools/edit_monitor/Android.bp index 80437c00d4..b939633817 100644 --- a/tools/edit_monitor/Android.bp +++ b/tools/edit_monitor/Android.bp @@ -19,3 +19,26 @@ package { default_applicable_licenses: ["Android-Apache-2.0"], default_team: "trendy_team_adte", } + +python_library_host { + name: "edit_monitor_lib", + pkg_path: "edit_monitor", + srcs: [ + "daemon_manager.py", + ], +} + +python_test_host { + name: "daemon_manager_test", + main: "daemon_manager_test.py", + pkg_path: "edit_monitor", + srcs: [ + "daemon_manager_test.py", + ], + libs: [ + "edit_monitor_lib", + ], + test_options: { + unit_test: true, + }, +} diff --git a/tools/edit_monitor/daemon_manager.py b/tools/edit_monitor/daemon_manager.py new file mode 100644 index 0000000000..8ec25886dc --- /dev/null +++ b/tools/edit_monitor/daemon_manager.py @@ -0,0 +1,182 @@ +# Copyright 2024, 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. + + +import hashlib +import logging +import multiprocessing +import os +import pathlib +import signal +import subprocess +import tempfile +import time + + +DEFAULT_PROCESS_TERMINATION_TIMEOUT_SECONDS = 1 + + +def default_daemon_target(): + """Place holder for the default daemon target.""" + print("default daemon target") + + +class DaemonManager: + """Class to manage and monitor the daemon run as a subprocess.""" + + def __init__( + self, + binary_path: str, + daemon_target: callable = default_daemon_target, + daemon_args: tuple = (), + ): + self.binary_path = binary_path + self.daemon_target = daemon_target + self.daemon_args = daemon_args + + self.pid = os.getpid() + self.daemon_process = None + + pid_file_dir = pathlib.Path(tempfile.gettempdir()).joinpath("edit_monitor") + pid_file_dir.mkdir(parents=True, exist_ok=True) + self.pid_file_path = self._get_pid_file_path(pid_file_dir) + + def start(self): + """Writes the pidfile and starts the daemon proces.""" + try: + self._stop_any_existing_instance() + self._write_pid_to_pidfile() + self._start_daemon_process() + except Exception as e: + logging.exception("Failed to start daemon manager with error %s", e) + + def stop(self): + """Stops the daemon process and removes the pidfile.""" + + logging.debug("in daemon manager cleanup.") + try: + if self.daemon_process and self.daemon_process.is_alive(): + self._terminate_process(self.daemon_process.pid) + self._remove_pidfile() + except Exception as e: + logging.exception("Failed to stop daemon manager with error %s", e) + + def _stop_any_existing_instance(self): + if not self.pid_file_path.exists(): + logging.debug("No existing instances.") + return + + ex_pid = self._read_pid_from_pidfile() + + if ex_pid: + logging.info("Found another instance with pid %d.", ex_pid) + self._terminate_process(ex_pid) + self._remove_pidfile() + + def _read_pid_from_pidfile(self): + with open(self.pid_file_path, "r") as f: + return int(f.read().strip()) + + def _write_pid_to_pidfile(self): + """Creates a pidfile and writes the current pid to the file. + + Raise FileExistsError if the pidfile already exists. + """ + try: + # Use the 'x' mode to open the file for exclusive creation + with open(self.pid_file_path, "x") as f: + f.write(f"{self.pid}") + except FileExistsError as e: + # This could be caused due to race condition that a user is trying + # to start two edit monitors at the same time. Or because there is + # already an existing edit monitor running and we can not kill it + # for some reason. + logging.exception("pidfile %s already exists.", self.pid_file_path) + raise e + + def _start_daemon_process(self): + """Starts a subprocess to run the daemon.""" + p = multiprocessing.Process( + target=self.daemon_target, args=self.daemon_args + ) + p.start() + + logging.info("Start subprocess with PID %d", p.pid) + self.daemon_process = p + + def _terminate_process( + self, pid: int, timeout: int = DEFAULT_PROCESS_TERMINATION_TIMEOUT_SECONDS + ): + """Terminates a process with given pid. + + It first sends a SIGTERM to the process to allow it for proper + termination with a timeout. If the process is not terminated within + the timeout, kills it forcefully. + """ + try: + os.kill(pid, signal.SIGTERM) + if not self._wait_for_process_terminate(pid, timeout): + logging.warning( + "Process %d not terminated within timeout, try force kill", pid + ) + os.kill(pid, signal.SIGKILL) + except ProcessLookupError: + logging.info("Process with PID %d not found (already terminated)", pid) + + def _wait_for_process_terminate(self, pid: int, timeout: int) -> bool: + start_time = time.time() + + while time.time() < start_time + timeout: + if not self._is_process_alive(pid): + return True + time.sleep(1) + + logging.error("Process %d not terminated within %d seconds.", pid, timeout) + return False + + def _is_process_alive(self, pid: int) -> bool: + try: + output = subprocess.check_output( + ["ps", "-p", str(pid), "-o", "state="], text=True + ).strip() + state = output.split()[0] + return state != "Z" # Check if the state is not 'Z' (zombie) + except subprocess.CalledProcessError: + # Process not found (already dead). + return False + except (FileNotFoundError, OSError, ValueError) as e: + logging.warning( + "Unable to check the status for process %d with error: %s.", pid, e + ) + return True + + def _remove_pidfile(self): + try: + os.remove(self.pid_file_path) + except FileNotFoundError: + logging.info("pid file %s already removed.", self.pid_file_path) + + def _get_pid_file_path(self, pid_file_dir: pathlib.Path) -> pathlib.Path: + """Generates the path to store the pidfile. + + The file path should have the format of "/tmp/edit_monitor/xxxx.lock" + where xxxx is a hashed value based on the binary path that starts the + process. + """ + hash_object = hashlib.sha256() + hash_object.update(self.binary_path.encode("utf-8")) + pid_file_path = pid_file_dir.joinpath(hash_object.hexdigest() + ".lock") + logging.info("pid_file_path: %s", pid_file_path) + + return pid_file_path diff --git a/tools/edit_monitor/daemon_manager_test.py b/tools/edit_monitor/daemon_manager_test.py new file mode 100644 index 0000000000..214b0388dc --- /dev/null +++ b/tools/edit_monitor/daemon_manager_test.py @@ -0,0 +1,253 @@ +# Copyright 2024, 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. + +"""Unittests for DaemonManager.""" + +import logging +import multiprocessing +import os +import pathlib +import signal +import subprocess +import sys +import tempfile +import time +import unittest +from unittest import mock +from edit_monitor import daemon_manager + +TEST_BINARY_FILE = '/path/to/test_binary' +TEST_PID_FILE_PATH = ( + '587239c2d1050afdf54512e2d799f3b929f86b43575eb3c7b4bab105dd9bd25e.lock' +) + + +def simple_daemon(output_file): + with open(output_file, 'w') as f: + f.write('running daemon target') + + +def long_running_daemon(): + while True: + time.sleep(1) + + +class DaemonManagerTest(unittest.TestCase): + + @classmethod + def setUpClass(cls): + super().setUpClass() + # Configure to print logging to stdout. + logging.basicConfig(filename=None, level=logging.DEBUG) + console = logging.StreamHandler(sys.stdout) + logging.getLogger('').addHandler(console) + + def setUp(self): + super().setUp() + self.original_tempdir = tempfile.tempdir + self.working_dir = tempfile.TemporaryDirectory() + # Sets the tempdir under the working dir so any temp files created during + # tests will be cleaned. + tempfile.tempdir = self.working_dir.name + + def tearDown(self): + # Cleans up any child processes left by the tests. + self._cleanup_child_processes() + self.working_dir.cleanup() + # Restores tempdir. + tempfile.tempdir = self.original_tempdir + super().tearDown() + + def test_start_success_with_no_existing_instance(self): + self.assert_run_simple_daemon_success() + + def test_start_success_with_existing_instance_running(self): + # Create a long running subprocess + p = multiprocessing.Process(target=long_running_daemon) + p.start() + + # Create a pidfile with the subprocess pid + pid_file_path_dir = pathlib.Path(self.working_dir.name).joinpath( + 'edit_monitor' + ) + pid_file_path_dir.mkdir(parents=True, exist_ok=True) + with open(pid_file_path_dir.joinpath(TEST_PID_FILE_PATH), 'w') as f: + f.write(str(p.pid)) + + self.assert_run_simple_daemon_success() + p.terminate() + + def test_start_success_with_existing_instance_already_dead(self): + # Create a pidfile with pid that does not exist. + pid_file_path_dir = pathlib.Path(self.working_dir.name).joinpath( + 'edit_monitor' + ) + pid_file_path_dir.mkdir(parents=True, exist_ok=True) + with open(pid_file_path_dir.joinpath(TEST_PID_FILE_PATH), 'w') as f: + f.write('123456') + + self.assert_run_simple_daemon_success() + + def test_start_success_with_existing_instance_from_different_binary(self): + # First start an instance based on "some_binary_path" + existing_dm = daemon_manager.DaemonManager( + "some_binary_path", + daemon_target=long_running_daemon, + ) + existing_dm.start() + + self.assert_run_simple_daemon_success() + existing_dm.stop() + + @mock.patch('os.kill') + def test_start_failed_to_kill_existing_instance(self, mock_kill): + mock_kill.side_effect = OSError('Unknown OSError') + pid_file_path_dir = pathlib.Path(self.working_dir.name).joinpath( + 'edit_monitor' + ) + pid_file_path_dir.mkdir(parents=True, exist_ok=True) + with open(pid_file_path_dir.joinpath(TEST_PID_FILE_PATH), 'w') as f: + f.write('123456') + + dm = daemon_manager.DaemonManager(TEST_BINARY_FILE) + dm.start() + + # Verify no daemon process is started. + self.assertIsNone(dm.daemon_process) + + def test_start_failed_to_write_pidfile(self): + pid_file_path_dir = pathlib.Path(self.working_dir.name).joinpath( + 'edit_monitor' + ) + pid_file_path_dir.mkdir(parents=True, exist_ok=True) + # Makes the directory read-only so write pidfile will fail. + os.chmod(pid_file_path_dir, 0o555) + + dm = daemon_manager.DaemonManager(TEST_BINARY_FILE) + dm.start() + + # Verifies no daemon process is started. + self.assertIsNone(dm.daemon_process) + + def test_start_failed_to_start_daemon_process(self): + dm = daemon_manager.DaemonManager( + TEST_BINARY_FILE, daemon_target='wrong_target', daemon_args=(1) + ) + dm.start() + + # Verifies no daemon process is started. + self.assertIsNone(dm.daemon_process) + + def test_stop_success(self): + dm = daemon_manager.DaemonManager( + TEST_BINARY_FILE, daemon_target=long_running_daemon + ) + dm.start() + dm.stop() + + self.assert_no_subprocess_running() + self.assertFalse(dm.pid_file_path.exists()) + + @mock.patch('os.kill') + def test_stop_failed_to_kill_daemon_process(self, mock_kill): + mock_kill.side_effect = OSError('Unknown OSError') + dm = daemon_manager.DaemonManager( + TEST_BINARY_FILE, daemon_target=long_running_daemon + ) + dm.start() + dm.stop() + + self.assertTrue(dm.daemon_process.is_alive()) + self.assertTrue(dm.pid_file_path.exists()) + + @mock.patch('os.remove') + def test_stop_failed_to_remove_pidfile(self, mock_remove): + mock_remove.side_effect = OSError('Unknown OSError') + + dm = daemon_manager.DaemonManager( + TEST_BINARY_FILE, daemon_target=long_running_daemon + ) + dm.start() + dm.stop() + + self.assert_no_subprocess_running() + self.assertTrue(dm.pid_file_path.exists()) + + def assert_run_simple_daemon_success(self): + damone_output_file = tempfile.NamedTemporaryFile( + dir=self.working_dir.name, delete=False + ) + dm = daemon_manager.DaemonManager( + TEST_BINARY_FILE, + daemon_target=simple_daemon, + daemon_args=(damone_output_file.name,), + ) + dm.start() + dm.daemon_process.join() + + # Verifies the expected pid file is created. + expected_pid_file_path = pathlib.Path(self.working_dir.name).joinpath( + 'edit_monitor', TEST_PID_FILE_PATH + ) + self.assertTrue(expected_pid_file_path.exists()) + + # Verify the daemon process is executed successfully. + with open(damone_output_file.name, 'r') as f: + contents = f.read() + self.assertEqual(contents, 'running daemon target') + + def assert_no_subprocess_running(self): + child_pids = self._get_child_processes(os.getpid()) + for child_pid in child_pids: + self.assertFalse( + self._is_process_alive(child_pid), f'process {child_pid} still alive' + ) + + def _get_child_processes(self, parent_pid): + try: + output = subprocess.check_output( + ['ps', '-o', 'pid,ppid', '--no-headers'], text=True + ) + + child_processes = [] + for line in output.splitlines(): + pid, ppid = line.split() + if int(ppid) == parent_pid: + child_processes.append(int(pid)) + return child_processes + except subprocess.CalledProcessError as e: + self.fail(f'failed to get child process, error: {e}') + + def _is_process_alive(self, pid): + try: + output = subprocess.check_output( + ['ps', '-p', str(pid), '-o', 'state='], text=True + ).strip() + state = output.split()[0] + return state != 'Z' # Check if the state is not 'Z' (zombie) + except subprocess.CalledProcessError: + return False + + def _cleanup_child_processes(self): + child_pids = self._get_child_processes(os.getpid()) + for child_pid in child_pids: + try: + os.kill(child_pid, signal.SIGKILL) + except ProcessLookupError: + # process already terminated + pass + + +if __name__ == '__main__': + unittest.main() |