Remove RO disabled flags before calc. fingerprint.
Move into helper fn as well so logic is consolidated.
Test: atest, manual
Change-Id: Iec6db2f51228d36c89c842e615fd845fe4c206a1
diff --git a/tools/aconfig/aconfig/src/codegen/cpp.rs b/tools/aconfig/aconfig/src/codegen/cpp.rs
index ae18679..d7d77c5 100644
--- a/tools/aconfig/aconfig/src/codegen/cpp.rs
+++ b/tools/aconfig/aconfig/src/codegen/cpp.rs
@@ -24,7 +24,7 @@
use crate::codegen;
use crate::codegen::CodegenMode;
-use crate::commands::OutputFile;
+use crate::commands::{should_include_flag, OutputFile};
pub fn generate_cpp_code<I>(
package: &str,
@@ -127,10 +127,7 @@
flag_ids: HashMap<String, u16>,
rw_count: &mut i32,
) -> ClassElement {
- let no_assigned_offset =
- (pf.container() == "system" || pf.container() == "vendor" || pf.container() == "product")
- && pf.permission() == ProtoFlagPermission::READ_ONLY
- && pf.state() == ProtoFlagState::DISABLED;
+ let no_assigned_offset = !should_include_flag(pf);
let flag_offset = match flag_ids.get(pf.name()) {
Some(offset) => offset,
diff --git a/tools/aconfig/aconfig/src/codegen/java.rs b/tools/aconfig/aconfig/src/codegen/java.rs
index 0d528d2..81340f2 100644
--- a/tools/aconfig/aconfig/src/codegen/java.rs
+++ b/tools/aconfig/aconfig/src/codegen/java.rs
@@ -22,7 +22,7 @@
use crate::codegen;
use crate::codegen::CodegenMode;
-use crate::commands::OutputFile;
+use crate::commands::{should_include_flag, OutputFile};
use aconfig_protos::{ProtoFlagPermission, ProtoFlagState, ProtoParsedFlag};
use std::collections::HashMap;
@@ -162,10 +162,7 @@
let device_config_flag = codegen::create_device_config_ident(package, pf.name())
.expect("values checked at flag parse time");
- let no_assigned_offset =
- (pf.container() == "system" || pf.container() == "vendor" || pf.container() == "product")
- && pf.permission() == ProtoFlagPermission::READ_ONLY
- && pf.state() == ProtoFlagState::DISABLED;
+ let no_assigned_offset = !should_include_flag(pf);
let flag_offset = match flag_offsets.get(pf.name()) {
Some(offset) => offset,
diff --git a/tools/aconfig/aconfig/src/codegen/rust.rs b/tools/aconfig/aconfig/src/codegen/rust.rs
index 2bf565a..74da1bc 100644
--- a/tools/aconfig/aconfig/src/codegen/rust.rs
+++ b/tools/aconfig/aconfig/src/codegen/rust.rs
@@ -24,7 +24,7 @@
use crate::codegen;
use crate::codegen::CodegenMode;
-use crate::commands::OutputFile;
+use crate::commands::{should_include_flag, OutputFile};
pub fn generate_rust_code<I>(
package: &str,
@@ -88,18 +88,12 @@
impl TemplateParsedFlag {
#[allow(clippy::nonminimal_bool)]
fn new(package: &str, flag_offsets: HashMap<String, u16>, pf: &ProtoParsedFlag) -> Self {
- let no_assigned_offset = (pf.container() == "system"
- || pf.container() == "vendor"
- || pf.container() == "product")
- && pf.permission() == ProtoFlagPermission::READ_ONLY
- && pf.state() == ProtoFlagState::DISABLED;
-
let flag_offset = match flag_offsets.get(pf.name()) {
Some(offset) => offset,
None => {
// System/vendor/product RO+disabled flags have no offset in storage files.
// Assign placeholder value.
- if no_assigned_offset {
+ if !should_include_flag(pf) {
&0
}
// All other flags _must_ have an offset.
diff --git a/tools/aconfig/aconfig/src/commands.rs b/tools/aconfig/aconfig/src/commands.rs
index 547076f..2f96015 100644
--- a/tools/aconfig/aconfig/src/commands.rs
+++ b/tools/aconfig/aconfig/src/commands.rs
@@ -221,13 +221,13 @@
new_exported: bool,
) -> Result<Vec<OutputFile>> {
let parsed_flags = input.try_parse_flags()?;
- let modified_parsed_flags = modify_parsed_flags_based_on_mode(parsed_flags, codegen_mode)?;
+ let modified_parsed_flags =
+ modify_parsed_flags_based_on_mode(parsed_flags.clone(), codegen_mode)?;
let Some(package) = find_unique_package(&modified_parsed_flags) else {
bail!("no parsed flags, or the parsed flags use different packages");
};
let package = package.to_string();
- let mut flag_names =
- modified_parsed_flags.iter().map(|pf| pf.name().to_string()).collect::<Vec<_>>();
+ let mut flag_names = extract_flag_names(parsed_flags)?;
let package_fingerprint = compute_flags_fingerprint(&mut flag_names);
let flag_ids = assign_flag_ids(&package, modified_parsed_flags.iter())?;
generate_java_code(
@@ -436,14 +436,7 @@
return Err(anyhow::anyhow!("the number of flags in a package cannot exceed 65535"));
}
- // Exclude system/vendor/product flags that are RO+disabled.
- let should_filter_container = pf.container == Some("vendor".to_string())
- || pf.container == Some("system".to_string())
- || pf.container == Some("product".to_string());
- if !(should_filter_container
- && pf.state == Some(ProtoFlagState::DISABLED.into())
- && pf.permission == Some(ProtoFlagPermission::READ_ONLY.into()))
- {
+ if should_include_flag(pf) {
flag_ids.insert(pf.name().to_string(), flag_idx as u16);
flag_idx += 1;
}
@@ -451,10 +444,8 @@
Ok(flag_ids)
}
-#[allow(dead_code)] // TODO: b/316357686 - Use fingerprint in codegen to
- // protect hardcoded offset reads.
- // Creates a fingerprint of the flag names (which requires sorting the vector).
- // Fingerprint is used by both codegen and storage files.
+// Creates a fingerprint of the flag names (which requires sorting the vector).
+// Fingerprint is used by both codegen and storage files.
pub fn compute_flags_fingerprint(flag_names: &mut Vec<String>) -> u64 {
flag_names.sort();
@@ -465,11 +456,9 @@
hasher.finish()
}
-#[allow(dead_code)] // TODO: b/316357686 - Use fingerprint in codegen to
- // protect hardcoded offset reads.
- // Converts ProtoParsedFlags into a vector of strings containing all of the flag
- // names. Helper fn for creating fingerprint for codegen files. Flags must all
- // belong to the same package.
+// Converts ProtoParsedFlags into a vector of strings containing all of the flag
+// names. Helper fn for creating fingerprint for codegen files. Flags must all
+// belong to the same package.
fn extract_flag_names(flags: ProtoParsedFlags) -> Result<Vec<String>> {
let separated_flags: Vec<ProtoParsedFlag> = flags.parsed_flag.into_iter().collect::<Vec<_>>();
@@ -478,7 +467,23 @@
bail!("No parsed flags, or the parsed flags use different packages.");
};
- Ok(separated_flags.into_iter().map(|flag| flag.name.unwrap()).collect::<Vec<_>>())
+ Ok(separated_flags
+ .into_iter()
+ .filter(should_include_flag)
+ .map(|flag| flag.name.unwrap())
+ .collect::<Vec<_>>())
+}
+
+// Exclude system/vendor/product flags that are RO+disabled.
+pub fn should_include_flag(pf: &ProtoParsedFlag) -> bool {
+ let should_filter_container = pf.container == Some("vendor".to_string())
+ || pf.container == Some("system".to_string())
+ || pf.container == Some("product".to_string());
+
+ let disabled_ro = pf.state == Some(ProtoFlagState::DISABLED.into())
+ && pf.permission == Some(ProtoFlagPermission::READ_ONLY.into());
+
+ !should_filter_container || !disabled_ro
}
#[cfg(test)]
@@ -489,7 +494,7 @@
#[test]
fn test_offset_fingerprint() {
let parsed_flags = crate::test::parse_test_flags();
- let expected_fingerprint: u64 = 5801144784618221668;
+ let expected_fingerprint: u64 = 11551379960324242360;
let mut extracted_flags = extract_flag_names(parsed_flags).unwrap();
let hash_result = compute_flags_fingerprint(&mut extracted_flags);
@@ -509,6 +514,7 @@
.parsed_flag
.clone()
.into_iter()
+ .filter(should_include_flag)
.map(|flag| flag.name.unwrap())
.map(String::from)
.collect::<Vec<_>>();
diff --git a/tools/aconfig/aconfig/src/storage/flag_table.rs b/tools/aconfig/aconfig/src/storage/flag_table.rs
index 3b245a7..a3b4e8f 100644
--- a/tools/aconfig/aconfig/src/storage/flag_table.rs
+++ b/tools/aconfig/aconfig/src/storage/flag_table.rs
@@ -14,9 +14,9 @@
* limitations under the License.
*/
-use crate::commands::assign_flag_ids;
+use crate::commands::{assign_flag_ids, should_include_flag};
use crate::storage::FlagPackage;
-use aconfig_protos::{ProtoFlagPermission, ProtoFlagState};
+use aconfig_protos::ProtoFlagPermission;
use aconfig_storage_file::{
get_table_size, FlagTable, FlagTableHeader, FlagTableNode, StorageFileType, StoredFlagType,
};
@@ -64,13 +64,7 @@
fn create_nodes(package: &FlagPackage, num_buckets: u32) -> Result<Vec<Self>> {
// Exclude system/vendor/product flags that are RO+disabled.
let mut filtered_package = package.clone();
- filtered_package.boolean_flags.retain(|f| {
- !((f.container == Some("system".to_string())
- || f.container == Some("vendor".to_string())
- || f.container == Some("product".to_string()))
- && f.permission == Some(ProtoFlagPermission::READ_ONLY.into())
- && f.state == Some(ProtoFlagState::DISABLED.into()))
- });
+ filtered_package.boolean_flags.retain(|pf| should_include_flag(pf));
let flag_ids =
assign_flag_ids(package.package_name, filtered_package.boolean_flags.iter().copied())?;