Merge "aconfig: add flag type in flag table and remove info byte from value array" into main

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<HashMap<String, u32>>
+pub fn assign_flag_ids<'a, I>(package: &str, parsed_flags_iter: I) -> Result<HashMap<String, u16>>
 where
     I: Iterator<Item = &'a ProtoParsedFlag> + 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_ro"), 0_u16),
-            (String::from("disabled_rw"), 1_u32),
+            (String::from("disabled_rw"), 1_u16),
-            (String::from("disabled_rw_exported"), 2_u32),
+            (String::from("disabled_rw_exported"), 2_u16),
-            (String::from("disabled_rw_in_other_namespace"), 3_u32),
+            (String::from("disabled_rw_in_other_namespace"), 3_u16),
-            (String::from("enabled_fixed_ro"), 4_u32),
+            (String::from("enabled_fixed_ro"), 4_u16),
-            (String::from("enabled_fixed_ro_exported"), 5_u32),
+            (String::from("enabled_fixed_ro_exported"), 5_u16),
-            (String::from("enabled_ro"), 6_u32),
+            (String::from("enabled_ro"), 6_u16),
-            (String::from("enabled_ro_exported"), 7_u32),
+            (String::from("enabled_ro_exported"), 7_u16),
-            (String::from("enabled_rw"), 8_u32),
+            (String::from("enabled_rw"), 8_u16),
         ]);
         assert_eq!(flag_ids, expected_flag_ids);
     }

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<u32>,
     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
@@ -106,7 +115,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::<Result<Vec<_>>>()
     }
@@ -175,7 +188,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 {
@@ -200,7 +213,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),
@@ -216,13 +230,15 @@ mod tests {
         fn new_expected(
             package_id: u32,
             flag_name: &str,
-            flag_id: u32,
+            flag_type: u16,
+            flag_id: u16,
             next_offset: Option<u32>,
             bucket_index: u32,
         ) -> Self {
             Self {
                 package_id,
                 flag_name: flag_name.to_string(),
+                flag_type,
                 flag_id,
                 next_offset,
                 bucket_index,
@@ -306,14 +322,17 @@ mod tests {
         let nodes: &Vec<FlagTableNode> = &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[0], FlagTableNode::new_expected(0, "enabled_ro", 1, 1, None, 0));
-        assert_eq!(nodes[1], FlagTableNode::new_expected(0, "enabled_rw", 2, Some(150), 1));
+        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", 0, None, 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, None, 5));
+        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, Some(235), 7));
+        assert_eq!(
-        assert_eq!(nodes[5], FlagTableNode::new_expected(1, "enabled_ro", 2, None, 7));
+            nodes[4],
-        assert_eq!(nodes[6], FlagTableNode::new_expected(2, "enabled_fixed_ro", 0, None, 9));
+            FlagTableNode::new_expected(1, "enabled_fixed_ro", 1, 1, Some(235), 7)
-        assert_eq!(nodes[7], FlagTableNode::new_expected(0, "disabled_rw", 0, None, 15));
+        );
+        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]

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
+    // is stored as a single byte
-    // byte is flag info byte, which is a bitmask to indicate the status of a flag
     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<u16> {
+        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<u32> {
         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);
     }
 }

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<u32>,
     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,
         };