aconfig: remove bucket_index from PackageTableNode/FlagTableNode struct
bucket index currently is a field in PackageTableNode/FlagTableNode, but this is
purely aux info that is never searilized or deserialized. Therefore we
should remove it from the struct definition. Instead aconfig should
define a wrapper struct that wraps around an instance PackageTableNode/FlagTableNode
as well as aux info like bucket_index.
Bug: 321077378
Test: atest aconfig.test && atest aconfig_storage_file.test
Change-Id: I20f2565d20b7feb5d39754e91cd6a9affb1f0e70
diff --git a/tools/aconfig/aconfig/src/storage/flag_table.rs b/tools/aconfig/aconfig/src/storage/flag_table.rs
index bebac890..4dd177c 100644
--- a/tools/aconfig/aconfig/src/storage/flag_table.rs
+++ b/tools/aconfig/aconfig/src/storage/flag_table.rs
@@ -32,40 +32,58 @@
}
}
-fn new_node(
- package_id: u32,
- flag_name: &str,
- flag_type: u16,
- flag_id: u16,
- num_buckets: u32,
-) -> FlagTableNode {
- let bucket_index = FlagTableNode::find_bucket_index(package_id, flag_name, num_buckets);
- FlagTableNode {
- package_id,
- flag_name: flag_name.to_string(),
- flag_type,
- flag_id,
- next_offset: None,
- bucket_index,
- }
+// a struct that contains FlagTableNode and a bunch of other information to help
+// flag table creation
+#[derive(PartialEq, Debug, Clone)]
+struct FlagTableNodeWrapper {
+ pub node: FlagTableNode,
+ pub bucket_index: u32,
}
-fn create_nodes(package: &FlagPackage, num_buckets: u32) -> Result<Vec<FlagTableNode>> {
- let flag_ids = assign_flag_ids(package.package_name, package.boolean_flags.iter().copied())?;
- package
- .boolean_flags
- .iter()
- .map(|&pf| {
- let fid = flag_ids
- .get(pf.name())
- .ok_or(anyhow!(format!("missing flag id for {}", pf.name())))?;
- // 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(new_node(package.package_id, pf.name(), flag_type, *fid, num_buckets))
- })
- .collect::<Result<Vec<_>>>()
+impl FlagTableNodeWrapper {
+ fn new(
+ package_id: u32,
+ flag_name: &str,
+ flag_type: u16,
+ flag_id: u16,
+ num_buckets: u32,
+ ) -> Self {
+ let bucket_index = FlagTableNode::find_bucket_index(package_id, flag_name, num_buckets);
+ let node = FlagTableNode {
+ package_id,
+ flag_name: flag_name.to_string(),
+ flag_type,
+ flag_id,
+ next_offset: None,
+ };
+ Self { node, bucket_index }
+ }
+
+ fn create_nodes(package: &FlagPackage, num_buckets: u32) -> Result<Vec<Self>> {
+ let flag_ids =
+ assign_flag_ids(package.package_name, package.boolean_flags.iter().copied())?;
+ package
+ .boolean_flags
+ .iter()
+ .map(|&pf| {
+ let fid = flag_ids
+ .get(pf.name())
+ .ok_or(anyhow!(format!("missing flag id for {}", pf.name())))?;
+ // 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(Self::new(
+ package.package_id,
+ pf.name(),
+ flag_type,
+ *fid,
+ num_buckets,
+ ))
+ })
+ .collect::<Result<Vec<_>>>()
+ }
}
pub fn create_flag_table(container: &str, packages: &[FlagPackage]) -> Result<FlagTable> {
@@ -73,44 +91,48 @@
let num_flags = packages.iter().map(|pkg| pkg.boolean_flags.len() as u32).sum();
let num_buckets = get_table_size(num_flags)?;
- let mut table = FlagTable {
- header: new_header(container, num_flags),
- buckets: vec![None; num_buckets as usize],
- nodes: packages
- .iter()
- .map(|pkg| create_nodes(pkg, num_buckets))
- .collect::<Result<Vec<_>>>()?
- .concat(),
- };
+ let mut header = new_header(container, num_flags);
+ let mut buckets = vec![None; num_buckets as usize];
+ let mut node_wrappers = packages
+ .iter()
+ .map(|pkg| FlagTableNodeWrapper::create_nodes(pkg, num_buckets))
+ .collect::<Result<Vec<_>>>()?
+ .concat();
// initialize all header fields
- table.header.bucket_offset = table.header.as_bytes().len() as u32;
- table.header.node_offset = table.header.bucket_offset + num_buckets * 4;
- table.header.file_size = table.header.node_offset
- + table.nodes.iter().map(|x| x.as_bytes().len()).sum::<usize>() as u32;
+ header.bucket_offset = header.as_bytes().len() as u32;
+ header.node_offset = header.bucket_offset + num_buckets * 4;
+ header.file_size = header.node_offset
+ + node_wrappers.iter().map(|x| x.node.as_bytes().len()).sum::<usize>() as u32;
// sort nodes by bucket index for efficiency
- table.nodes.sort_by(|a, b| a.bucket_index.cmp(&b.bucket_index));
+ node_wrappers.sort_by(|a, b| a.bucket_index.cmp(&b.bucket_index));
// fill all node offset
- let mut offset = table.header.node_offset;
- for i in 0..table.nodes.len() {
- let node_bucket_idx = table.nodes[i].bucket_index;
+ let mut offset = header.node_offset;
+ for i in 0..node_wrappers.len() {
+ let node_bucket_idx = node_wrappers[i].bucket_index;
let next_node_bucket_idx =
- if i + 1 < table.nodes.len() { Some(table.nodes[i + 1].bucket_index) } else { None };
+ if i + 1 < node_wrappers.len() { Some(node_wrappers[i + 1].bucket_index) } else { None };
- if table.buckets[node_bucket_idx as usize].is_none() {
- table.buckets[node_bucket_idx as usize] = Some(offset);
+ if buckets[node_bucket_idx as usize].is_none() {
+ buckets[node_bucket_idx as usize] = Some(offset);
}
- offset += table.nodes[i].as_bytes().len() as u32;
+ offset += node_wrappers[i].node.as_bytes().len() as u32;
if let Some(index) = next_node_bucket_idx {
if index == node_bucket_idx {
- table.nodes[i].next_offset = Some(offset);
+ node_wrappers[i].node.next_offset = Some(offset);
}
}
}
+ let table = FlagTable {
+ header,
+ buckets,
+ nodes: node_wrappers.into_iter().map(|nw| nw.node).collect(),
+ };
+
Ok(table)
}
@@ -126,7 +148,6 @@
flag_type: u16,
flag_id: u16,
next_offset: Option<u32>,
- bucket_index: u32,
) -> FlagTableNode {
FlagTableNode {
package_id,
@@ -134,7 +155,6 @@
flag_type,
flag_id,
next_offset,
- bucket_index,
}
}
@@ -186,13 +206,13 @@
let nodes: &Vec<FlagTableNode> = &flag_table.as_ref().unwrap().nodes;
assert_eq!(nodes.len(), 8);
- assert_eq!(nodes[0], new_expected_node(0, "enabled_ro", 1, 1, None, 0));
- assert_eq!(nodes[1], new_expected_node(0, "enabled_rw", 1, 2, Some(150), 1));
- assert_eq!(nodes[2], new_expected_node(1, "disabled_ro", 1, 0, None, 1));
- assert_eq!(nodes[3], new_expected_node(2, "enabled_ro", 1, 1, None, 5));
- assert_eq!(nodes[4], new_expected_node(1, "enabled_fixed_ro", 1, 1, Some(235), 7));
- assert_eq!(nodes[5], new_expected_node(1, "enabled_ro", 1, 2, None, 7));
- assert_eq!(nodes[6], new_expected_node(2, "enabled_fixed_ro", 1, 0, None, 9));
- assert_eq!(nodes[7], new_expected_node(0, "disabled_rw", 1, 0, None, 15));
+ assert_eq!(nodes[0], new_expected_node(0, "enabled_ro", 1, 1, None));
+ assert_eq!(nodes[1], new_expected_node(0, "enabled_rw", 1, 2, Some(150)));
+ assert_eq!(nodes[2], new_expected_node(1, "disabled_ro", 1, 0, None));
+ assert_eq!(nodes[3], new_expected_node(2, "enabled_ro", 1, 1, None));
+ assert_eq!(nodes[4], new_expected_node(1, "enabled_fixed_ro", 1, 1, Some(235)));
+ assert_eq!(nodes[5], new_expected_node(1, "enabled_ro", 1, 2, None));
+ assert_eq!(nodes[6], new_expected_node(2, "enabled_fixed_ro", 1, 0, None));
+ assert_eq!(nodes[7], new_expected_node(0, "disabled_rw", 1, 0, None));
}
}
diff --git a/tools/aconfig/aconfig/src/storage/package_table.rs b/tools/aconfig/aconfig/src/storage/package_table.rs
index f82e932..5ce6165 100644
--- a/tools/aconfig/aconfig/src/storage/package_table.rs
+++ b/tools/aconfig/aconfig/src/storage/package_table.rs
@@ -17,7 +17,7 @@
use anyhow::Result;
use aconfig_storage_file::{
- get_bucket_index, get_table_size, PackageTable, PackageTableHeader, PackageTableNode,
+ get_table_size, PackageTable, PackageTableHeader, PackageTableNode,
FILE_VERSION,
};
@@ -34,14 +34,24 @@
}
}
-fn new_node(package: &FlagPackage, num_buckets: u32) -> PackageTableNode {
- let bucket_index = get_bucket_index(&package.package_name.to_string(), num_buckets);
- PackageTableNode {
- package_name: String::from(package.package_name),
- package_id: package.package_id,
- boolean_offset: package.boolean_offset,
- next_offset: None,
- bucket_index,
+// a struct that contains PackageTableNode and a bunch of other information to help
+// package table creation
+#[derive(PartialEq, Debug)]
+struct PackageTableNodeWrapper {
+ pub node: PackageTableNode,
+ pub bucket_index: u32,
+}
+
+impl PackageTableNodeWrapper {
+ fn new(package: &FlagPackage, num_buckets: u32) -> Self {
+ let node = PackageTableNode {
+ package_name: String::from(package.package_name),
+ package_id: package.package_id,
+ boolean_offset: package.boolean_offset,
+ next_offset: None,
+ };
+ let bucket_index = PackageTableNode::find_bucket_index(package.package_name, num_buckets);
+ Self { node, bucket_index }
}
}
@@ -49,40 +59,46 @@
// create table
let num_packages = packages.len() as u32;
let num_buckets = get_table_size(num_packages)?;
- let mut table = PackageTable {
- header: new_header(container, num_packages),
- buckets: vec![None; num_buckets as usize],
- nodes: packages.iter().map(|pkg| new_node(pkg, num_buckets)).collect(),
- };
+ let mut header = new_header(container, num_packages);
+ let mut buckets = vec![None; num_buckets as usize];
+ let mut node_wrappers: Vec<_> = packages
+ .iter()
+ .map(|pkg| PackageTableNodeWrapper::new(pkg, num_buckets))
+ .collect();
// initialize all header fields
- table.header.bucket_offset = table.header.as_bytes().len() as u32;
- table.header.node_offset = table.header.bucket_offset + num_buckets * 4;
- table.header.file_size = table.header.node_offset
- + table.nodes.iter().map(|x| x.as_bytes().len()).sum::<usize>() as u32;
+ header.bucket_offset = header.as_bytes().len() as u32;
+ header.node_offset = header.bucket_offset + num_buckets * 4;
+ header.file_size = header.node_offset
+ + node_wrappers.iter().map(|x| x.node.as_bytes().len()).sum::<usize>() as u32;
- // sort nodes by bucket index for efficiency
- table.nodes.sort_by(|a, b| a.bucket_index.cmp(&b.bucket_index));
+ // sort node_wrappers by bucket index for efficiency
+ node_wrappers.sort_by(|a, b| a.bucket_index.cmp(&b.bucket_index));
// fill all node offset
- let mut offset = table.header.node_offset;
- for i in 0..table.nodes.len() {
- let node_bucket_idx = table.nodes[i].bucket_index;
+ let mut offset = header.node_offset;
+ for i in 0..node_wrappers.len() {
+ let node_bucket_idx = node_wrappers[i].bucket_index;
let next_node_bucket_idx =
- if i + 1 < table.nodes.len() { Some(table.nodes[i + 1].bucket_index) } else { None };
+ if i + 1 < node_wrappers.len() { Some(node_wrappers[i + 1].bucket_index) } else { None };
- if table.buckets[node_bucket_idx as usize].is_none() {
- table.buckets[node_bucket_idx as usize] = Some(offset);
+ if buckets[node_bucket_idx as usize].is_none() {
+ buckets[node_bucket_idx as usize] = Some(offset);
}
- offset += table.nodes[i].as_bytes().len() as u32;
+ offset += node_wrappers[i].node.as_bytes().len() as u32;
if let Some(index) = next_node_bucket_idx {
if index == node_bucket_idx {
- table.nodes[i].next_offset = Some(offset);
+ node_wrappers[i].node.next_offset = Some(offset);
}
}
}
+ let table = PackageTable {
+ header,
+ buckets,
+ nodes: node_wrappers.into_iter().map(|nw| nw.node).collect(),
+ };
Ok(table)
}
@@ -125,7 +141,6 @@
package_id: 1,
boolean_offset: 3,
next_offset: None,
- bucket_index: 0,
};
assert_eq!(nodes[0], first_node_expected);
let second_node_expected = PackageTableNode {
@@ -133,7 +148,6 @@
package_id: 0,
boolean_offset: 0,
next_offset: Some(158),
- bucket_index: 3,
};
assert_eq!(nodes[1], second_node_expected);
let third_node_expected = PackageTableNode {
@@ -141,7 +155,6 @@
package_id: 2,
boolean_offset: 6,
next_offset: None,
- bucket_index: 3,
};
assert_eq!(nodes[2], third_node_expected);
}
diff --git a/tools/aconfig/aconfig_storage_file/src/flag_table.rs b/tools/aconfig/aconfig_storage_file/src/flag_table.rs
index 421f847..99d5a60 100644
--- a/tools/aconfig/aconfig_storage_file/src/flag_table.rs
+++ b/tools/aconfig/aconfig_storage_file/src/flag_table.rs
@@ -68,7 +68,6 @@
pub flag_type: u16,
pub flag_id: u16,
pub next_offset: Option<u32>,
- pub bucket_index: u32,
}
impl FlagTableNode {
@@ -97,7 +96,6 @@
0 => None,
val => Some(val),
},
- bucket_index: 0,
};
Ok(node)
}
@@ -142,13 +140,11 @@
.collect();
let nodes = (0..num_flags)
.map(|_| {
- let mut node = FlagTableNode::from_bytes(&bytes[head..]).unwrap();
+ let node = FlagTableNode::from_bytes(&bytes[head..])?;
head += node.as_bytes().len();
- node.bucket_index = FlagTableNode::find_bucket_index(
- node.package_id, &node.flag_name, num_buckets);
- node
+ Ok(node)
})
- .collect();
+ .collect::<Result<Vec<_>>>()?;
let table = Self { header, buckets, nodes };
Ok(table)
@@ -203,7 +199,6 @@
flag_type: u16,
flag_id: u16,
next_offset: Option<u32>,
- bucket_index: u32,
) -> Self {
Self {
package_id,
@@ -211,7 +206,6 @@
flag_type,
flag_id,
next_offset,
- bucket_index,
}
}
}
@@ -245,14 +239,14 @@
None,
];
let nodes = vec![
- FlagTableNode::new_expected(0, "enabled_ro", 1, 1, None, 0),
- FlagTableNode::new_expected(0, "enabled_rw", 1, 2, Some(150), 1),
- FlagTableNode::new_expected(1, "disabled_ro", 1, 0, None, 1),
- FlagTableNode::new_expected(2, "enabled_ro", 1, 1, None, 5),
- FlagTableNode::new_expected(1, "enabled_fixed_ro", 1, 1, Some(235), 7),
- FlagTableNode::new_expected(1, "enabled_ro", 1, 2, None, 7),
- FlagTableNode::new_expected(2, "enabled_fixed_ro", 1, 0, None, 9),
- FlagTableNode::new_expected(0, "disabled_rw", 1, 0, None, 15),
+ FlagTableNode::new_expected(0, "enabled_ro", 1, 1, None),
+ FlagTableNode::new_expected(0, "enabled_rw", 1, 2, Some(150)),
+ FlagTableNode::new_expected(1, "disabled_ro", 1, 0, None),
+ FlagTableNode::new_expected(2, "enabled_ro", 1, 1, None),
+ FlagTableNode::new_expected(1, "enabled_fixed_ro", 1, 1, Some(235)),
+ FlagTableNode::new_expected(1, "enabled_ro", 1, 2, None),
+ FlagTableNode::new_expected(2, "enabled_fixed_ro", 1, 0, None),
+ FlagTableNode::new_expected(0, "disabled_rw", 1, 0, None),
];
Ok(FlagTable { header, buckets, nodes })
}
@@ -268,14 +262,8 @@
assert_eq!(header, &reinterpreted_header.unwrap());
let nodes: &Vec<FlagTableNode> = &flag_table.nodes;
- let num_buckets = crate::get_table_size(header.num_flags).unwrap();
for node in nodes.iter() {
- let mut reinterpreted_node = FlagTableNode::from_bytes(&node.as_bytes()).unwrap();
- reinterpreted_node.bucket_index = FlagTableNode::find_bucket_index(
- reinterpreted_node.package_id,
- &reinterpreted_node.flag_name,
- num_buckets
- );
+ let reinterpreted_node = FlagTableNode::from_bytes(&node.as_bytes()).unwrap();
assert_eq!(node, &reinterpreted_node);
}
diff --git a/tools/aconfig/aconfig_storage_file/src/package_table.rs b/tools/aconfig/aconfig_storage_file/src/package_table.rs
index 8fd57e6..a3ad6ec 100644
--- a/tools/aconfig/aconfig_storage_file/src/package_table.rs
+++ b/tools/aconfig/aconfig_storage_file/src/package_table.rs
@@ -69,7 +69,6 @@
// boolean flag value array in the flag value file
pub boolean_offset: u32,
pub next_offset: Option<u32>,
- pub bucket_index: u32,
}
impl PackageTableNode {
@@ -96,10 +95,16 @@
0 => None,
val => Some(val),
},
- bucket_index: 0,
};
Ok(node)
}
+
+ /// Get the bucket index for a package table node, defined it here so the
+ /// construction side (aconfig binary) and consumption side (flag read lib)
+ /// use the same method of hashing
+ pub fn find_bucket_index(package: &str, num_buckets: u32) -> u32 {
+ get_bucket_index(&package, num_buckets)
+ }
}
/// Package table struct
@@ -135,12 +140,11 @@
.collect();
let nodes = (0..num_packages)
.map(|_| {
- let mut node = PackageTableNode::from_bytes(&bytes[head..]).unwrap();
+ let node = PackageTableNode::from_bytes(&bytes[head..])?;
head += node.as_bytes().len();
- node.bucket_index = get_bucket_index(&node.package_name, num_buckets);
- node
+ Ok(node)
})
- .collect();
+ .collect::<Result<Vec<_>>>()?;
let table = Self { header, buckets, nodes };
Ok(table)
@@ -166,7 +170,7 @@
}
let num_buckets = (interpreted_header.node_offset - interpreted_header.bucket_offset) / 4;
- let bucket_index = get_bucket_index(&package, num_buckets);
+ let bucket_index = PackageTableNode::find_bucket_index(&package, num_buckets);
let mut pos = (interpreted_header.bucket_offset + 4 * bucket_index) as usize;
let mut package_node_offset = read_u32_from_bytes(buf, &mut pos)? as usize;
@@ -210,21 +214,18 @@
package_id: 1,
boolean_offset: 3,
next_offset: None,
- bucket_index: 0,
};
let second_node = PackageTableNode {
package_name: String::from("com.android.aconfig.storage.test_1"),
package_id: 0,
boolean_offset: 0,
next_offset: Some(158),
- bucket_index: 3,
};
let third_node = PackageTableNode {
package_name: String::from("com.android.aconfig.storage.test_4"),
package_id: 2,
boolean_offset: 6,
next_offset: None,
- bucket_index: 3,
};
let nodes = vec![first_node, second_node, third_node];
Ok(PackageTable { header, buckets, nodes })
@@ -241,11 +242,8 @@
assert_eq!(header, &reinterpreted_header.unwrap());
let nodes: &Vec<PackageTableNode> = &package_table.nodes;
- let num_buckets = crate::get_table_size(header.num_packages).unwrap();
for node in nodes.iter() {
- let mut reinterpreted_node = PackageTableNode::from_bytes(&node.as_bytes()).unwrap();
- reinterpreted_node.bucket_index =
- get_bucket_index(&reinterpreted_node.package_name, num_buckets);
+ let reinterpreted_node = PackageTableNode::from_bytes(&node.as_bytes()).unwrap();
assert_eq!(node, &reinterpreted_node);
}