From d088650481a69fc908c4f2bd438c591b08a86911 Mon Sep 17 00:00:00 2001 From: Dennis Shen Date: Tue, 9 Jan 2024 16:49:53 +0000 Subject: [PATCH] aconfig: add flag type in flag table and remove info byte from value array 1, add flag type to the flag table. Before flag table only stores the mapping from (package id, flag name) to (flag id u32). The original intent is to do bitmasking on the top byte of flag id to indicate flag type. Now split the flag id u32 to two u16, the first represent flag type, the second represent flag id. So after the change, the flag table now shows the following mapping: (package id, flag name) -> (flag type as u16, flag id as u16) 2, originally we plan to store a info byte together with each flag value. The info byte is used by storage service damemon to mark up the flag status, such as if it is accepting server side flag push. After internal discussion, it is better to just create the info bytes as another file by storage service damemon. So that the value file is purely a flag value array. Bug: b/312243587 test: atest aconfig.test Change-Id: I7f953076b4269cf786bc23723078290e5ebe10bc --- tools/aconfig/src/commands.rs | 28 ++++++++----- tools/aconfig/src/storage/flag_table.rs | 47 +++++++++++++++------- tools/aconfig/src/storage/mod.rs | 18 ++++++--- tools/aconfig/src/storage/package_table.rs | 6 ++- 4 files changed, 67 insertions(+), 32 deletions(-) diff --git a/tools/aconfig/src/commands.rs b/tools/aconfig/src/commands.rs index 1a8872bc3e..f7a641776c 100644 --- a/tools/aconfig/src/commands.rs +++ b/tools/aconfig/src/commands.rs @@ -361,7 +361,7 @@ pub fn modify_parsed_flags_based_on_mode( Ok(modified_parsed_flags) } -pub fn assign_flag_ids<'a, I>(package: &str, parsed_flags_iter: I) -> Result> +pub fn assign_flag_ids<'a, I>(package: &str, parsed_flags_iter: I) -> Result> where I: Iterator + Clone, { @@ -371,7 +371,13 @@ where if package != pf.package() { return Err(anyhow::anyhow!("encountered a flag not in current package")); } - flag_ids.insert(pf.name().to_string(), id_to_assign); + + // put a cap on how many flags a package can contain to 65535 + if id_to_assign > u16::MAX as u32 { + return Err(anyhow::anyhow!("the number of flags in a package cannot exceed 65535")); + } + + flag_ids.insert(pf.name().to_string(), id_to_assign as u16); } Ok(flag_ids) } @@ -693,15 +699,15 @@ mod tests { 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_flag_ids = HashMap::from([ - (String::from("disabled_ro"), 0_u32), - (String::from("disabled_rw"), 1_u32), - (String::from("disabled_rw_exported"), 2_u32), - (String::from("disabled_rw_in_other_namespace"), 3_u32), - (String::from("enabled_fixed_ro"), 4_u32), - (String::from("enabled_fixed_ro_exported"), 5_u32), - (String::from("enabled_ro"), 6_u32), - (String::from("enabled_ro_exported"), 7_u32), - (String::from("enabled_rw"), 8_u32), + (String::from("disabled_ro"), 0_u16), + (String::from("disabled_rw"), 1_u16), + (String::from("disabled_rw_exported"), 2_u16), + (String::from("disabled_rw_in_other_namespace"), 3_u16), + (String::from("enabled_fixed_ro"), 4_u16), + (String::from("enabled_fixed_ro_exported"), 5_u16), + (String::from("enabled_ro"), 6_u16), + (String::from("enabled_ro_exported"), 7_u16), + (String::from("enabled_rw"), 8_u16), ]); assert_eq!(flag_ids, expected_flag_ids); } diff --git a/tools/aconfig/src/storage/flag_table.rs b/tools/aconfig/src/storage/flag_table.rs index 46753f0ad6..5cfe3a5fcb 100644 --- a/tools/aconfig/src/storage/flag_table.rs +++ b/tools/aconfig/src/storage/flag_table.rs @@ -58,18 +58,26 @@ impl FlagTableHeader { pub struct FlagTableNode { pub package_id: u32, pub flag_name: String, - pub flag_id: u32, + pub flag_type: u16, + pub flag_id: u16, pub next_offset: Option, pub bucket_index: u32, } impl FlagTableNode { - fn new(package_id: u32, flag_name: &str, flag_id: u32, num_buckets: u32) -> Self { + fn new( + package_id: u32, + flag_name: &str, + flag_type: u16, + flag_id: u16, + num_buckets: u32, + ) -> Self { let full_flag_name = package_id.to_string() + "/" + flag_name; let bucket_index = storage::get_bucket_index(&full_flag_name, num_buckets); Self { package_id, flag_name: flag_name.to_string(), + flag_type, flag_id, next_offset: None, bucket_index, @@ -82,6 +90,7 @@ impl FlagTableNode { let name_bytes = self.flag_name.as_bytes(); result.extend_from_slice(&(name_bytes.len() as u32).to_le_bytes()); result.extend_from_slice(name_bytes); + result.extend_from_slice(&self.flag_type.to_le_bytes()); result.extend_from_slice(&self.flag_id.to_le_bytes()); result.extend_from_slice(&self.next_offset.unwrap_or(0).to_le_bytes()); result @@ -108,7 +117,11 @@ impl FlagTable { let fid = flag_ids .get(pf.name()) .ok_or(anyhow!(format!("missing flag id for {}", pf.name())))?; - Ok(FlagTableNode::new(package.package_id, pf.name(), *fid, num_buckets)) + // all flags are boolean value at the moment, thus using the last bit. When more + // flag value types are supported, flag value type information should come from the + // parsed flag, and we will set the flag_type bit mask properly. + let flag_type = 1; + Ok(FlagTableNode::new(package.package_id, pf.name(), flag_type, *fid, num_buckets)) }) .collect::>>() } @@ -177,7 +190,7 @@ mod tests { use super::*; use crate::storage::{ group_flags_by_package, tests::parse_all_test_flags, tests::read_str_from_bytes, - tests::read_u32_from_bytes, + tests::read_u16_from_bytes, tests::read_u32_from_bytes, }; impl FlagTableHeader { @@ -202,7 +215,8 @@ mod tests { let mut node = Self { package_id: read_u32_from_bytes(bytes, &mut head)?, flag_name: read_str_from_bytes(bytes, &mut head)?, - flag_id: read_u32_from_bytes(bytes, &mut head)?, + flag_type: read_u16_from_bytes(bytes, &mut head)?, + flag_id: read_u16_from_bytes(bytes, &mut head)?, next_offset: match read_u32_from_bytes(bytes, &mut head)? { 0 => None, val => Some(val), @@ -218,13 +232,15 @@ mod tests { fn new_expected( package_id: u32, flag_name: &str, - flag_id: u32, + flag_type: u16, + flag_id: u16, next_offset: Option, bucket_index: u32, ) -> Self { Self { package_id, flag_name: flag_name.to_string(), + flag_type, flag_id, next_offset, bucket_index, @@ -308,14 +324,17 @@ mod tests { let nodes: &Vec = &flag_table.as_ref().unwrap().nodes; assert_eq!(nodes.len(), 8); - assert_eq!(nodes[0], FlagTableNode::new_expected(0, "enabled_ro", 1, None, 0)); - assert_eq!(nodes[1], FlagTableNode::new_expected(0, "enabled_rw", 2, Some(150), 1)); - assert_eq!(nodes[2], FlagTableNode::new_expected(1, "disabled_ro", 0, None, 1)); - assert_eq!(nodes[3], FlagTableNode::new_expected(2, "enabled_ro", 1, None, 5)); - assert_eq!(nodes[4], FlagTableNode::new_expected(1, "enabled_fixed_ro", 1, Some(235), 7)); - assert_eq!(nodes[5], FlagTableNode::new_expected(1, "enabled_ro", 2, None, 7)); - assert_eq!(nodes[6], FlagTableNode::new_expected(2, "enabled_fixed_ro", 0, None, 9)); - assert_eq!(nodes[7], FlagTableNode::new_expected(0, "disabled_rw", 0, None, 15)); + assert_eq!(nodes[0], FlagTableNode::new_expected(0, "enabled_ro", 1, 1, None, 0)); + assert_eq!(nodes[1], FlagTableNode::new_expected(0, "enabled_rw", 1, 2, Some(150), 1)); + assert_eq!(nodes[2], FlagTableNode::new_expected(1, "disabled_ro", 1, 0, None, 1)); + assert_eq!(nodes[3], FlagTableNode::new_expected(2, "enabled_ro", 1, 1, None, 5)); + assert_eq!( + nodes[4], + FlagTableNode::new_expected(1, "enabled_fixed_ro", 1, 1, Some(235), 7) + ); + assert_eq!(nodes[5], FlagTableNode::new_expected(1, "enabled_ro", 1, 2, None, 7)); + assert_eq!(nodes[6], FlagTableNode::new_expected(2, "enabled_fixed_ro", 1, 0, None, 9)); + assert_eq!(nodes[7], FlagTableNode::new_expected(0, "disabled_rw", 1, 0, None, 15)); } #[test] diff --git a/tools/aconfig/src/storage/mod.rs b/tools/aconfig/src/storage/mod.rs index 76835e015d..a28fccd917 100644 --- a/tools/aconfig/src/storage/mod.rs +++ b/tools/aconfig/src/storage/mod.rs @@ -56,6 +56,8 @@ pub struct FlagPackage<'a> { pub package_id: u32, pub flag_names: HashSet<&'a str>, pub boolean_flags: Vec<&'a ProtoParsedFlag>, + // offset of the first boolean flag in this flag package with respect to the start of + // boolean flag value array in the flag value file pub boolean_offset: u32, } @@ -95,12 +97,11 @@ where } // calculate package flag value start offset, in flag value file, each boolean - // is stored as two bytes, the first byte will be the flag value. the second - // byte is flag info byte, which is a bitmask to indicate the status of a flag + // is stored as a single byte let mut boolean_offset = 0; for p in packages.iter_mut() { p.boolean_offset = boolean_offset; - boolean_offset += 2 * p.boolean_flags.len() as u32; + boolean_offset += p.boolean_flags.len() as u32; } packages @@ -135,6 +136,13 @@ mod tests { use super::*; use crate::Input; + /// Read and parse bytes as u16 + pub fn read_u16_from_bytes(buf: &[u8], head: &mut usize) -> Result { + let val = u16::from_le_bytes(buf[*head..*head + 2].try_into()?); + *head += 2; + Ok(val) + } + /// Read and parse bytes as u32 pub fn read_u32_from_bytes(buf: &[u8], head: &mut usize) -> Result { let val = u32::from_le_bytes(buf[*head..*head + 4].try_into()?); @@ -218,13 +226,13 @@ mod tests { assert!(packages[1].flag_names.contains("enabled_ro")); assert!(packages[1].flag_names.contains("disabled_ro")); assert!(packages[1].flag_names.contains("enabled_fixed_ro")); - assert_eq!(packages[1].boolean_offset, 6); + assert_eq!(packages[1].boolean_offset, 3); assert_eq!(packages[2].package_name, "com.android.aconfig.storage.test_4"); assert_eq!(packages[2].package_id, 2); assert_eq!(packages[2].flag_names.len(), 2); assert!(packages[2].flag_names.contains("enabled_ro")); assert!(packages[2].flag_names.contains("enabled_fixed_ro")); - assert_eq!(packages[2].boolean_offset, 12); + assert_eq!(packages[2].boolean_offset, 6); } } diff --git a/tools/aconfig/src/storage/package_table.rs b/tools/aconfig/src/storage/package_table.rs index 1a3bbc3eb0..0ce13493b5 100644 --- a/tools/aconfig/src/storage/package_table.rs +++ b/tools/aconfig/src/storage/package_table.rs @@ -57,6 +57,8 @@ impl PackageTableHeader { pub struct PackageTableNode { pub package_name: String, pub package_id: u32, + // offset of the first boolean flag in this flag package with respect to the start of + // boolean flag value array in the flag value file pub boolean_offset: u32, pub next_offset: Option, pub bucket_index: u32, @@ -249,7 +251,7 @@ mod tests { let first_node_expected = PackageTableNode { package_name: String::from("com.android.aconfig.storage.test_2"), package_id: 1, - boolean_offset: 6, + boolean_offset: 3, next_offset: None, bucket_index: 0, }; @@ -265,7 +267,7 @@ mod tests { let third_node_expected = PackageTableNode { package_name: String::from("com.android.aconfig.storage.test_4"), package_id: 2, - boolean_offset: 12, + boolean_offset: 6, next_offset: None, bucket_index: 3, };