aconfig: use an enum to represetn flag type
Flag type is encoded as a u16 in flag table file. Previously we are
assinging value of 1 for flag type to indicate that they are all boolean
flags. In this change, we formalize the flag type designation with an
enum.
ReadWriteBoolean -> 0
ReadOnlyBoolean -> 1
FixedReadOnlyBoolean -> 2
Bug: b/321077378
Test: atest aconfg.test; atest aconfig_storage_file.test
Change-Id: I114818435b44f1e82a611f0509787993a5a8e70e
diff --git a/tools/aconfig/aconfig/src/storage/flag_table.rs b/tools/aconfig/aconfig/src/storage/flag_table.rs
index 401379b..a07f179 100644
--- a/tools/aconfig/aconfig/src/storage/flag_table.rs
+++ b/tools/aconfig/aconfig/src/storage/flag_table.rs
@@ -16,8 +16,10 @@
use crate::commands::assign_flag_ids;
use crate::storage::FlagPackage;
+use aconfig_protos::ProtoFlagPermission;
use aconfig_storage_file::{
- get_table_size, FlagTable, FlagTableHeader, FlagTableNode, StorageFileType, FILE_VERSION,
+ get_table_size, FlagTable, FlagTableHeader, FlagTableNode, StorageFileType, StoredFlagType,
+ FILE_VERSION,
};
use anyhow::{anyhow, Result};
@@ -45,7 +47,7 @@
fn new(
package_id: u32,
flag_name: &str,
- flag_type: u16,
+ flag_type: StoredFlagType,
flag_id: u16,
num_buckets: u32,
) -> Self {
@@ -70,11 +72,14 @@
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;
+ let flag_type = if pf.is_fixed_read_only() {
+ StoredFlagType::FixedReadOnlyBoolean
+ } else {
+ match pf.permission() {
+ ProtoFlagPermission::READ_WRITE => StoredFlagType::ReadWriteBoolean,
+ ProtoFlagPermission::READ_ONLY => StoredFlagType::ReadOnlyBoolean,
+ }
+ };
Ok(Self::new(package.package_id, pf.name(), flag_type, *fid, num_buckets))
})
.collect::<Result<Vec<_>>>()
@@ -147,7 +152,7 @@
FlagTableNode {
package_id,
flag_name: flag_name.to_string(),
- flag_type,
+ flag_type: StoredFlagType::try_from(flag_type).unwrap(),
flag_id,
next_offset,
}
@@ -203,12 +208,12 @@
assert_eq!(nodes.len(), 8);
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(151)));
+ assert_eq!(nodes[1], new_expected_node(0, "enabled_rw", 0, 2, Some(151)));
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(236)));
+ assert_eq!(nodes[4], new_expected_node(1, "enabled_fixed_ro", 2, 1, Some(236)));
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));
+ assert_eq!(nodes[6], new_expected_node(2, "enabled_fixed_ro", 2, 0, None));
+ assert_eq!(nodes[7], new_expected_node(0, "disabled_rw", 0, 0, None));
}
}
diff --git a/tools/aconfig/aconfig/src/storage/flag_value.rs b/tools/aconfig/aconfig/src/storage/flag_value.rs
index c5cf215..4dcf751 100644
--- a/tools/aconfig/aconfig/src/storage/flag_value.rs
+++ b/tools/aconfig/aconfig/src/storage/flag_value.rs
@@ -88,7 +88,7 @@
assert_eq!(header, &expected_header);
let booleans: &Vec<bool> = &flag_value_list.as_ref().unwrap().booleans;
- let expected_booleans: Vec<bool> = vec![false; header.num_flags as usize];
+ let expected_booleans: Vec<bool> = vec![false, true, true, false, true, true, true, true];
assert_eq!(booleans, &expected_booleans);
}
}
diff --git a/tools/aconfig/aconfig/src/storage/mod.rs b/tools/aconfig/aconfig/src/storage/mod.rs
index 11f1a43..30517de 100644
--- a/tools/aconfig/aconfig/src/storage/mod.rs
+++ b/tools/aconfig/aconfig/src/storage/mod.rs
@@ -122,30 +122,38 @@
"com.android.aconfig.storage.test_1",
"storage_test_1.aconfig",
include_bytes!("../../tests/storage_test_1.aconfig").as_slice(),
+ "storage_test_1.value",
+ include_bytes!("../../tests/storage_test_1.values").as_slice(),
),
(
"com.android.aconfig.storage.test_2",
"storage_test_2.aconfig",
include_bytes!("../../tests/storage_test_2.aconfig").as_slice(),
+ "storage_test_2.value",
+ include_bytes!("../../tests/storage_test_2.values").as_slice(),
),
(
"com.android.aconfig.storage.test_4",
"storage_test_4.aconfig",
include_bytes!("../../tests/storage_test_4.aconfig").as_slice(),
+ "storage_test_4.value",
+ include_bytes!("../../tests/storage_test_4.values").as_slice(),
),
];
-
aconfig_files
.into_iter()
- .map(|(pkg, file, content)| {
+ .map(|(pkg, aconfig_file, aconfig_content, value_file, value_content)| {
let bytes = crate::commands::parse_flags(
pkg,
Some("system"),
vec![Input {
- source: format!("tests/{}", file).to_string(),
- reader: Box::new(content),
+ source: format!("tests/{}", aconfig_file).to_string(),
+ reader: Box::new(aconfig_content),
}],
- vec![],
+ vec![Input {
+ source: format!("tests/{}", value_file).to_string(),
+ reader: Box::new(value_content),
+ }],
crate::commands::DEFAULT_FLAG_PERMISSION,
)
.unwrap();
diff --git a/tools/aconfig/aconfig/tests/storage_test_1.values b/tools/aconfig/aconfig/tests/storage_test_1.values
new file mode 100644
index 0000000..35548ae
--- /dev/null
+++ b/tools/aconfig/aconfig/tests/storage_test_1.values
@@ -0,0 +1,18 @@
+flag_value {
+ package: "com.android.aconfig.storage.test_1"
+ name: "enabled_rw"
+ state: ENABLED
+ permission: READ_WRITE
+}
+flag_value {
+ package: "com.android.aconfig.storage.test_1"
+ name: "disabled_rw"
+ state: DISABLED
+ permission: READ_WRITE
+}
+flag_value {
+ package: "com.android.aconfig.storage.test_1"
+ name: "enabled_ro"
+ state: ENABLED
+ permission: READ_ONLY
+}
diff --git a/tools/aconfig/aconfig/tests/storage_test_2.values b/tools/aconfig/aconfig/tests/storage_test_2.values
new file mode 100644
index 0000000..a7bb0b1
--- /dev/null
+++ b/tools/aconfig/aconfig/tests/storage_test_2.values
@@ -0,0 +1,18 @@
+flag_value {
+ package: "com.android.aconfig.storage.test_2"
+ name: "enabled_ro"
+ state: ENABLED
+ permission: READ_ONLY
+}
+flag_value {
+ package: "com.android.aconfig.storage.test_2"
+ name: "disabled_ro"
+ state: DISABLED
+ permission: READ_ONLY
+}
+flag_value {
+ package: "com.android.aconfig.storage.test_2"
+ name: "enabled_fixed_ro"
+ state: ENABLED
+ permission: READ_ONLY
+}
diff --git a/tools/aconfig/aconfig/tests/storage_test_4.values b/tools/aconfig/aconfig/tests/storage_test_4.values
new file mode 100644
index 0000000..fa21317
--- /dev/null
+++ b/tools/aconfig/aconfig/tests/storage_test_4.values
@@ -0,0 +1,12 @@
+flag_value {
+ package: "com.android.aconfig.storage.test_4"
+ name: "enabled_ro"
+ state: ENABLED
+ permission: READ_ONLY
+}
+flag_value {
+ package: "com.android.aconfig.storage.test_4"
+ name: "enabled_fixed_ro"
+ state: ENABLED
+ permission: READ_ONLY
+}
diff --git a/tools/aconfig/aconfig_storage_file/src/flag_table.rs b/tools/aconfig/aconfig_storage_file/src/flag_table.rs
index 2e2b857..98a1321 100644
--- a/tools/aconfig/aconfig_storage_file/src/flag_table.rs
+++ b/tools/aconfig/aconfig_storage_file/src/flag_table.rs
@@ -21,7 +21,7 @@
get_bucket_index, read_str_from_bytes, read_u16_from_bytes, read_u32_from_bytes,
read_u8_from_bytes,
};
-use crate::{AconfigStorageError, StorageFileType};
+use crate::{AconfigStorageError, StorageFileType, StoredFlagType};
use anyhow::anyhow;
use std::fmt;
@@ -99,7 +99,7 @@
pub struct FlagTableNode {
pub package_id: u32,
pub flag_name: String,
- pub flag_type: u16,
+ pub flag_type: StoredFlagType,
pub flag_id: u16,
pub next_offset: Option<u32>,
}
@@ -109,7 +109,7 @@
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
writeln!(
f,
- "Package Id: {}, Flag: {}, Type: {}, Offset: {}, Next: {:?}",
+ "Package Id: {}, Flag: {}, Type: {:?}, Offset: {}, Next: {:?}",
self.package_id, self.flag_name, self.flag_type, self.flag_id, self.next_offset
)?;
Ok(())
@@ -124,7 +124,7 @@
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_type as u16).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
@@ -136,7 +136,7 @@
let node = Self {
package_id: read_u32_from_bytes(bytes, &mut head)?,
flag_name: read_str_from_bytes(bytes, &mut head)?,
- flag_type: read_u16_from_bytes(bytes, &mut head)?,
+ flag_type: StoredFlagType::try_from(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,
diff --git a/tools/aconfig/aconfig_storage_file/src/lib.rs b/tools/aconfig/aconfig_storage_file/src/lib.rs
index e7fda8a..a47926f 100644
--- a/tools/aconfig/aconfig_storage_file/src/lib.rs
+++ b/tools/aconfig/aconfig_storage_file/src/lib.rs
@@ -52,7 +52,7 @@
pub use crate::flag_value::{FlagValueHeader, FlagValueList};
pub use crate::package_table::{PackageTable, PackageTableHeader, PackageTableNode};
-use crate::AconfigStorageError::{BytesParseFail, HashTableSizeLimit};
+use crate::AconfigStorageError::{BytesParseFail, HashTableSizeLimit, InvalidStoredFlagType};
/// Storage file version
pub const FILE_VERSION: u32 = 1;
@@ -83,7 +83,7 @@
"flag_val" => Ok(Self::FlagVal),
"flag_info" => Ok(Self::FlagInfo),
_ => Err(anyhow!(
- "Invalid storage file type, valid types are package_map|flag_map|flag_val"
+ "Invalid storage file type, valid types are package_map|flag_map|flag_val|flag_info"
)),
}
}
@@ -103,6 +103,27 @@
}
}
+/// Flag type enum as stored by storage file
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub enum StoredFlagType {
+ ReadWriteBoolean = 0,
+ ReadOnlyBoolean = 1,
+ FixedReadOnlyBoolean = 2,
+}
+
+impl TryFrom<u16> for StoredFlagType {
+ type Error = AconfigStorageError;
+
+ fn try_from(value: u16) -> Result<Self, Self::Error> {
+ match value {
+ x if x == Self::ReadWriteBoolean as u16 => Ok(Self::ReadWriteBoolean),
+ x if x == Self::ReadOnlyBoolean as u16 => Ok(Self::ReadOnlyBoolean),
+ x if x == Self::FixedReadOnlyBoolean as u16 => Ok(Self::FixedReadOnlyBoolean),
+ _ => Err(InvalidStoredFlagType(anyhow!("Invalid stored flag type"))),
+ }
+ }
+}
+
/// Get the right hash table size given number of entries in the table. Use a
/// load factor of 0.5 for performance.
pub fn get_table_size(entries: u32) -> Result<u32, AconfigStorageError> {
@@ -201,6 +222,9 @@
#[error("failed to create file")]
FileCreationFail(#[source] anyhow::Error),
+
+ #[error("invalid stored flag type")]
+ InvalidStoredFlagType(#[source] anyhow::Error),
}
/// Read in storage file as bytes
diff --git a/tools/aconfig/aconfig_storage_file/src/test_utils.rs b/tools/aconfig/aconfig_storage_file/src/test_utils.rs
index 4641829..4097ba6 100644
--- a/tools/aconfig/aconfig_storage_file/src/test_utils.rs
+++ b/tools/aconfig/aconfig_storage_file/src/test_utils.rs
@@ -18,7 +18,7 @@
use crate::flag_table::{FlagTable, FlagTableHeader, FlagTableNode};
use crate::flag_value::{FlagValueHeader, FlagValueList};
use crate::package_table::{PackageTable, PackageTableHeader, PackageTableNode};
-use crate::{AconfigStorageError, StorageFileType};
+use crate::{AconfigStorageError, StorageFileType, StoredFlagType};
use anyhow::anyhow;
use std::io::Write;
@@ -66,7 +66,13 @@
flag_id: u16,
next_offset: Option<u32>,
) -> Self {
- Self { package_id, flag_name: flag_name.to_string(), flag_type, flag_id, next_offset }
+ Self {
+ package_id,
+ flag_name: flag_name.to_string(),
+ flag_type: StoredFlagType::try_from(flag_type).unwrap(),
+ flag_id,
+ next_offset,
+ }
}
}
diff --git a/tools/aconfig/aconfig_storage_read_api/src/flag_table_query.rs b/tools/aconfig/aconfig_storage_read_api/src/flag_table_query.rs
index 4c19646..8ce2397 100644
--- a/tools/aconfig/aconfig_storage_read_api/src/flag_table_query.rs
+++ b/tools/aconfig/aconfig_storage_read_api/src/flag_table_query.rs
@@ -65,7 +65,7 @@
#[cfg(test)]
mod tests {
use super::*;
- use aconfig_storage_file::{FlagTable, StorageFileType};
+ use aconfig_storage_file::{FlagTable, StorageFileType, StoredFlagType};
// create test baseline, syntactic sugar
fn new_expected_node(
@@ -78,7 +78,7 @@
FlagTableNode {
package_id,
flag_name: flag_name.to_string(),
- flag_type,
+ flag_type: StoredFlagType::try_from(flag_type).unwrap(),
flag_id,
next_offset,
}