diff options
-rw-r--r-- | tools/aconfig/aconfig/src/codegen/cpp.rs | 7 | ||||
-rw-r--r-- | tools/aconfig/aconfig/src/codegen/java.rs | 7 | ||||
-rw-r--r-- | tools/aconfig/aconfig/src/codegen/rust.rs | 10 | ||||
-rw-r--r-- | tools/aconfig/aconfig/src/commands.rs | 50 | ||||
-rw-r--r-- | tools/aconfig/aconfig/src/storage/flag_table.rs | 12 |
5 files changed, 37 insertions, 49 deletions
diff --git a/tools/aconfig/aconfig/src/codegen/cpp.rs b/tools/aconfig/aconfig/src/codegen/cpp.rs index ae18679f62..d7d77c5d7c 100644 --- a/tools/aconfig/aconfig/src/codegen/cpp.rs +++ b/tools/aconfig/aconfig/src/codegen/cpp.rs @@ -24,7 +24,7 @@ use aconfig_protos::{ProtoFlagPermission, ProtoFlagState, ProtoParsedFlag}; 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 @@ fn create_class_element( 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 0d528d2299..81340f29a1 100644 --- a/tools/aconfig/aconfig/src/codegen/java.rs +++ b/tools/aconfig/aconfig/src/codegen/java.rs @@ -22,7 +22,7 @@ use tinytemplate::TinyTemplate; 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 @@ fn create_flag_element( 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 2bf565a81c..74da1bcc32 100644 --- a/tools/aconfig/aconfig/src/codegen/rust.rs +++ b/tools/aconfig/aconfig/src/codegen/rust.rs @@ -24,7 +24,7 @@ use std::collections::HashMap; 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 @@ struct TemplateParsedFlag { 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 547076fb66..2f960151e1 100644 --- a/tools/aconfig/aconfig/src/commands.rs +++ b/tools/aconfig/aconfig/src/commands.rs @@ -221,13 +221,13 @@ pub fn create_java_lib( 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 @@ where 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 @@ where 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 @@ pub fn compute_flags_fingerprint(flag_names: &mut Vec<String>) -> u64 { 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 @@ fn extract_flag_names(flags: ProtoParsedFlags) -> Result<Vec<String>> { 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 @@ mod tests { #[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 @@ mod tests { .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 3b245a76f2..a3b4e8fe1e 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 @@ impl FlagTableNodeWrapper { 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())?; |