From 71f1b35fb48b1a748f1f6b458328fb9884d03047 Mon Sep 17 00:00:00 2001 From: Zhi Dou Date: Mon, 21 Aug 2023 22:49:46 +0000 Subject: [PATCH] aconfig: add fixed read only flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a new field in the declaration to indicate whether the permission can be overridden. When the field “is_fixed_read_only” is set to true, the flag permission will be set as fixed “READ_ONLY”, and the permission should not be changed by Gantry. Bug: 292521627 Test: atest aconfig.test Change-Id: Ic9bcd7823bccb8b947cf05568c7ced3763490a23 --- tools/aconfig/protos/aconfig.proto | 2 + tools/aconfig/src/codegen_cpp.rs | 56 +++++++++++++++++++++- tools/aconfig/src/codegen_java.rs | 19 ++++++++ tools/aconfig/src/codegen_rust.rs | 35 ++++++++++++++ tools/aconfig/src/commands.rs | 72 ++++++++++++++++++++++++++-- tools/aconfig/src/protos.rs | 5 ++ tools/aconfig/src/test.rs | 24 ++++++++++ tools/aconfig/tests/AconfigTest.java | 10 ++++ tools/aconfig/tests/first.values | 6 +++ tools/aconfig/tests/test.aconfig | 11 +++++ 10 files changed, 236 insertions(+), 4 deletions(-) diff --git a/tools/aconfig/protos/aconfig.proto b/tools/aconfig/protos/aconfig.proto index 4cad69ad5f..d5e286843d 100644 --- a/tools/aconfig/protos/aconfig.proto +++ b/tools/aconfig/protos/aconfig.proto @@ -39,6 +39,7 @@ message flag_declaration { optional string namespace = 2; optional string description = 3; repeated string bug = 4; + optional bool is_fixed_read_only = 5; }; message flag_declarations { @@ -75,6 +76,7 @@ message parsed_flag { optional flag_state state = 6; optional flag_permission permission = 7; repeated tracepoint trace = 8; + optional bool is_fixed_read_only = 9; } message parsed_flags { diff --git a/tools/aconfig/src/codegen_cpp.rs b/tools/aconfig/src/codegen_cpp.rs index 30e564a5a1..8c2d7ba07f 100644 --- a/tools/aconfig/src/codegen_cpp.rs +++ b/tools/aconfig/src/codegen_cpp.rs @@ -131,6 +131,8 @@ public: virtual bool disabled_rw() = 0; + virtual bool enabled_fixed_ro() = 0; + virtual bool enabled_ro() = 0; virtual bool enabled_rw() = 0; @@ -146,6 +148,10 @@ inline bool disabled_rw() { return provider_->disabled_rw(); } +inline bool enabled_fixed_ro() { + return true; +} + inline bool enabled_ro() { return true; } @@ -163,6 +169,8 @@ bool com_android_aconfig_test_disabled_ro(); bool com_android_aconfig_test_disabled_rw(); +bool com_android_aconfig_test_enabled_fixed_ro(); + bool com_android_aconfig_test_enabled_ro(); bool com_android_aconfig_test_enabled_rw(); @@ -194,6 +202,10 @@ public: virtual void disabled_rw(bool val) = 0; + virtual bool enabled_fixed_ro() = 0; + + virtual void enabled_fixed_ro(bool val) = 0; + virtual bool enabled_ro() = 0; virtual void enabled_ro(bool val) = 0; @@ -223,6 +235,14 @@ inline void disabled_rw(bool val) { provider_->disabled_rw(val); } +inline bool enabled_fixed_ro() { + return provider_->enabled_fixed_ro(); +} + +inline void enabled_fixed_ro(bool val) { + provider_->enabled_fixed_ro(val); +} + inline bool enabled_ro() { return provider_->enabled_ro(); } @@ -256,6 +276,10 @@ bool com_android_aconfig_test_disabled_rw(); void set_com_android_aconfig_test_disabled_rw(bool val); +bool com_android_aconfig_test_enabled_fixed_ro(); + +void set_com_android_aconfig_test_enabled_fixed_ro(bool val); + bool com_android_aconfig_test_enabled_ro(); void set_com_android_aconfig_test_enabled_ro(bool val); @@ -294,6 +318,10 @@ namespace com::android::aconfig::test { "false") == "true"; } + virtual bool enabled_fixed_ro() override { + return true; + } + virtual bool enabled_ro() override { return true; } @@ -319,6 +347,10 @@ bool com_android_aconfig_test_disabled_rw() { return com::android::aconfig::test::disabled_rw(); } +bool com_android_aconfig_test_enabled_fixed_ro() { + return true; +} + bool com_android_aconfig_test_enabled_ro() { return true; } @@ -373,6 +405,19 @@ namespace com::android::aconfig::test { overrides_["disabled_rw"] = val; } + virtual bool enabled_fixed_ro() override { + auto it = overrides_.find("enabled_fixed_ro"); + if (it != overrides_.end()) { + return it->second; + } else { + return true; + } + } + + virtual void enabled_fixed_ro(bool val) override { + overrides_["enabled_fixed_ro"] = val; + } + virtual bool enabled_ro() override { auto it = overrides_.find("enabled_ro"); if (it != overrides_.end()) { @@ -402,7 +447,6 @@ namespace com::android::aconfig::test { overrides_["enabled_rw"] = val; } - virtual void reset_flags() override { overrides_.clear(); } @@ -430,6 +474,16 @@ void set_com_android_aconfig_test_disabled_rw(bool val) { com::android::aconfig::test::disabled_rw(val); } + +bool com_android_aconfig_test_enabled_fixed_ro() { + return com::android::aconfig::test::enabled_fixed_ro(); +} + +void set_com_android_aconfig_test_enabled_fixed_ro(bool val) { + com::android::aconfig::test::enabled_fixed_ro(val); +} + + bool com_android_aconfig_test_enabled_ro() { return com::android::aconfig::test::enabled_ro(); } diff --git a/tools/aconfig/src/codegen_java.rs b/tools/aconfig/src/codegen_java.rs index 2c9dcf5a48..be0ec976b2 100644 --- a/tools/aconfig/src/codegen_java.rs +++ b/tools/aconfig/src/codegen_java.rs @@ -121,6 +121,7 @@ mod tests { public interface FeatureFlags { boolean disabledRo(); boolean disabledRw(); + boolean enabledFixedRo(); boolean enabledRo(); boolean enabledRw(); "#; @@ -130,6 +131,7 @@ mod tests { public final class Flags { public static final String FLAG_DISABLED_RO = "com.android.aconfig.test.disabled_ro"; public static final String FLAG_DISABLED_RW = "com.android.aconfig.test.disabled_rw"; + public static final String FLAG_ENABLED_FIXED_RO = "com.android.aconfig.test.enabled_fixed_ro"; public static final String FLAG_ENABLED_RO = "com.android.aconfig.test.enabled_ro"; public static final String FLAG_ENABLED_RW = "com.android.aconfig.test.enabled_rw"; @@ -139,6 +141,9 @@ mod tests { public static boolean disabledRw() { return FEATURE_FLAGS.disabledRw(); } + public static boolean enabledFixedRo() { + return FEATURE_FLAGS.enabledFixedRo(); + } public static boolean enabledRo() { return FEATURE_FLAGS.enabledRo(); } @@ -159,6 +164,11 @@ mod tests { "Method is not implemented."); } @Override + public boolean enabledFixedRo() { + throw new UnsupportedOperationException( + "Method is not implemented."); + } + @Override public boolean enabledRo() { throw new UnsupportedOperationException( "Method is not implemented."); @@ -211,6 +221,10 @@ mod tests { ); } @Override + public boolean enabledFixedRo() { + return true; + } + @Override public boolean enabledRo() { return true; } @@ -311,6 +325,10 @@ mod tests { return getFlag(Flags.FLAG_DISABLED_RW); } @Override + public boolean enabledFixedRo() { + return getFlag(Flags.FLAG_ENABLED_FIXED_RO); + } + @Override public boolean enabledRo() { return getFlag(Flags.FLAG_ENABLED_RO); } @@ -341,6 +359,7 @@ mod tests { private HashMap mFlagMap = Stream.of( Flags.FLAG_DISABLED_RO, Flags.FLAG_DISABLED_RW, + Flags.FLAG_ENABLED_FIXED_RO, Flags.FLAG_ENABLED_RO, Flags.FLAG_ENABLED_RW ) diff --git a/tools/aconfig/src/codegen_rust.rs b/tools/aconfig/src/codegen_rust.rs index 0234eb25d5..4e4c7dd58a 100644 --- a/tools/aconfig/src/codegen_rust.rs +++ b/tools/aconfig/src/codegen_rust.rs @@ -108,6 +108,11 @@ impl FlagProvider { "false") == "true" } + /// query flag enabled_fixed_ro + pub fn enabled_fixed_ro(&self) -> bool { + true + } + /// query flag enabled_ro pub fn enabled_ro(&self) -> bool { true @@ -137,6 +142,12 @@ pub fn disabled_rw() -> bool { PROVIDER.disabled_rw() } +/// query flag enabled_fixed_ro +#[inline(always)] +pub fn enabled_fixed_ro() -> bool { + true +} + /// query flag enabled_ro #[inline(always)] pub fn enabled_ro() -> bool { @@ -189,6 +200,18 @@ impl FlagProvider { self.overrides.insert("disabled_rw", val); } + /// query flag enabled_fixed_ro + pub fn enabled_fixed_ro(&self) -> bool { + self.overrides.get("enabled_fixed_ro").copied().unwrap_or( + true + ) + } + + /// set flag enabled_fixed_ro + pub fn set_enabled_fixed_ro(&mut self, val: bool) { + self.overrides.insert("enabled_fixed_ro", val); + } + /// query flag enabled_ro pub fn enabled_ro(&self) -> bool { self.overrides.get("enabled_ro").copied().unwrap_or( @@ -251,6 +274,18 @@ pub fn set_disabled_rw(val: bool) { PROVIDER.lock().unwrap().set_disabled_rw(val); } +/// query flag enabled_fixed_ro +#[inline(always)] +pub fn enabled_fixed_ro() -> bool { + PROVIDER.lock().unwrap().enabled_fixed_ro() +} + +/// set flag enabled_fixed_ro +#[inline(always)] +pub fn set_enabled_fixed_ro(val: bool) { + PROVIDER.lock().unwrap().set_enabled_fixed_ro(val); +} + /// query flag enabled_ro #[inline(always)] pub fn enabled_ro() -> bool { diff --git a/tools/aconfig/src/commands.rs b/tools/aconfig/src/commands.rs index ab5b0f2e25..e4baa82901 100644 --- a/tools/aconfig/src/commands.rs +++ b/tools/aconfig/src/commands.rs @@ -91,11 +91,17 @@ pub fn parse_flags( parsed_flag.set_description(flag_declaration.take_description()); parsed_flag.bug.append(&mut flag_declaration.bug); parsed_flag.set_state(DEFAULT_FLAG_STATE); - parsed_flag.set_permission(default_permission); + let flag_permission = if flag_declaration.is_fixed_read_only() { + ProtoFlagPermission::READ_ONLY + } else { + default_permission + }; + parsed_flag.set_permission(flag_permission); + parsed_flag.set_is_fixed_read_only(flag_declaration.is_fixed_read_only()); let mut tracepoint = ProtoTracepoint::new(); tracepoint.set_source(input.source.clone()); tracepoint.set_state(DEFAULT_FLAG_STATE); - tracepoint.set_permission(default_permission); + tracepoint.set_permission(flag_permission); parsed_flag.trace.push(tracepoint); // verify ParsedFlag looks reasonable @@ -135,6 +141,13 @@ pub fn parse_flags( continue; }; + ensure!( + !parsed_flag.is_fixed_read_only() + || flag_value.permission() == ProtoFlagPermission::READ_ONLY, + "failed to set permission of flag {}, since this flag is fixed read only flag", + flag_value.name() + ); + parsed_flag.set_state(flag_value.state()); parsed_flag.set_permission(flag_value.permission()); let mut tracepoint = ProtoTracepoint::new(); @@ -310,6 +323,7 @@ mod tests { assert_eq!(ProtoFlagState::ENABLED, enabled_ro.state()); assert_eq!(ProtoFlagPermission::READ_ONLY, enabled_ro.permission()); assert_eq!(3, enabled_ro.trace.len()); + assert!(!enabled_ro.is_fixed_read_only()); assert_eq!("tests/test.aconfig", enabled_ro.trace[0].source()); assert_eq!(ProtoFlagState::DISABLED, enabled_ro.trace[0].state()); assert_eq!(ProtoFlagPermission::READ_WRITE, enabled_ro.trace[0].permission()); @@ -320,8 +334,11 @@ mod tests { assert_eq!(ProtoFlagState::ENABLED, enabled_ro.trace[2].state()); assert_eq!(ProtoFlagPermission::READ_ONLY, enabled_ro.trace[2].permission()); - assert_eq!(4, parsed_flags.parsed_flag.len()); + assert_eq!(5, parsed_flags.parsed_flag.len()); for pf in parsed_flags.parsed_flag.iter() { + if pf.name() == "enabled_fixed_ro" { + continue; + } let first = pf.trace.first().unwrap(); assert_eq!(DEFAULT_FLAG_STATE, first.state()); assert_eq!(DEFAULT_FLAG_PERMISSION, first.permission()); @@ -330,6 +347,15 @@ mod tests { assert_eq!(pf.state(), last.state()); assert_eq!(pf.permission(), last.permission()); } + + let enabled_fixed_ro = + parsed_flags.parsed_flag.iter().find(|pf| pf.name() == "enabled_fixed_ro").unwrap(); + assert!(enabled_fixed_ro.is_fixed_read_only()); + assert_eq!(ProtoFlagState::ENABLED, enabled_fixed_ro.state()); + assert_eq!(ProtoFlagPermission::READ_ONLY, enabled_fixed_ro.permission()); + assert_eq!(2, enabled_fixed_ro.trace.len()); + assert_eq!(ProtoFlagPermission::READ_ONLY, enabled_fixed_ro.trace[0].permission()); + assert_eq!(ProtoFlagPermission::READ_ONLY, enabled_fixed_ro.trace[1].permission()); } #[test] @@ -362,6 +388,46 @@ mod tests { assert_eq!(ProtoFlagPermission::READ_ONLY, parsed_flag.permission()); } + #[test] + fn test_parse_flags_override_fixed_read_only() { + let first_flag = r#" + package: "com.first" + flag { + name: "first" + namespace: "first_ns" + description: "This is the description of the first flag." + bug: "123" + is_fixed_read_only: true + } + "#; + let declaration = + vec![Input { source: "memory".to_string(), reader: Box::new(first_flag.as_bytes()) }]; + + let first_flag_value = r#" + flag_value { + package: "com.first" + name: "first" + state: DISABLED + permission: READ_WRITE + } + "#; + let value = vec![Input { + source: "memory".to_string(), + reader: Box::new(first_flag_value.as_bytes()), + }]; + let error = crate::commands::parse_flags( + "com.first", + declaration, + value, + ProtoFlagPermission::READ_WRITE, + ) + .unwrap_err(); + assert_eq!( + format!("{:?}", error), + "failed to set permission of flag first, since this flag is fixed read only flag" + ); + } + #[test] fn test_create_device_config_defaults() { let input = parse_test_flags_as_input(); diff --git a/tools/aconfig/src/protos.rs b/tools/aconfig/src/protos.rs index c3911e52ac..b93abcce24 100644 --- a/tools/aconfig/src/protos.rs +++ b/tools/aconfig/src/protos.rs @@ -303,6 +303,7 @@ flag { namespace: "second_ns" description: "This is the description of the second flag." bug: "abc" + is_fixed_read_only: true } "#, ) @@ -313,11 +314,13 @@ flag { assert_eq!(first.namespace(), "first_ns"); assert_eq!(first.description(), "This is the description of the first flag."); assert_eq!(first.bug, vec!["123"]); + assert!(!first.is_fixed_read_only()); let second = flag_declarations.flag.iter().find(|pf| pf.name() == "second").unwrap(); assert_eq!(second.name(), "second"); assert_eq!(second.namespace(), "second_ns"); assert_eq!(second.description(), "This is the description of the second flag."); assert_eq!(second.bug, vec!["abc"]); + assert!(second.is_fixed_read_only()); // bad input: missing package in flag declarations let error = flag_declarations::try_from_text_proto( @@ -555,6 +558,7 @@ parsed_flag { state: ENABLED permission: READ_WRITE } + is_fixed_read_only: true } "#; let parsed_flags = try_from_binary_proto_from_text_proto(text_proto).unwrap(); @@ -574,6 +578,7 @@ parsed_flag { assert_eq!(second.trace[1].source(), "flags.values"); assert_eq!(second.trace[1].state(), ProtoFlagState::ENABLED); assert_eq!(second.trace[1].permission(), ProtoFlagPermission::READ_WRITE); + assert!(second.is_fixed_read_only()); // valid input: empty let parsed_flags = try_from_binary_proto_from_text_proto("").unwrap(); diff --git a/tools/aconfig/src/test.rs b/tools/aconfig/src/test.rs index 6c278850d4..9034704283 100644 --- a/tools/aconfig/src/test.rs +++ b/tools/aconfig/src/test.rs @@ -41,6 +41,7 @@ parsed_flag { state: DISABLED permission: READ_ONLY } + is_fixed_read_only: false } parsed_flag { package: "com.android.aconfig.test" @@ -55,6 +56,27 @@ parsed_flag { state: DISABLED permission: READ_WRITE } + is_fixed_read_only: false +} +parsed_flag { + package: "com.android.aconfig.test" + name: "enabled_fixed_ro" + namespace: "aconfig_test" + description: "This flag is fixed READ_ONLY + ENABLED" + bug: "" + state: ENABLED + permission: READ_ONLY + trace { + source: "tests/test.aconfig" + state: DISABLED + permission: READ_ONLY + } + trace { + source: "tests/first.values" + state: ENABLED + permission: READ_ONLY + } + is_fixed_read_only: true } parsed_flag { package: "com.android.aconfig.test" @@ -79,6 +101,7 @@ parsed_flag { state: ENABLED permission: READ_ONLY } + is_fixed_read_only: false } parsed_flag { package: "com.android.aconfig.test" @@ -98,6 +121,7 @@ parsed_flag { state: ENABLED permission: READ_WRITE } + is_fixed_read_only: false } "#; diff --git a/tools/aconfig/tests/AconfigTest.java b/tools/aconfig/tests/AconfigTest.java index 317289de3d..79b5a55d3c 100644 --- a/tools/aconfig/tests/AconfigTest.java +++ b/tools/aconfig/tests/AconfigTest.java @@ -1,9 +1,11 @@ import static com.android.aconfig.test.Flags.FLAG_DISABLED_RO; import static com.android.aconfig.test.Flags.FLAG_DISABLED_RW; +import static com.android.aconfig.test.Flags.FLAG_ENABLED_FIXED_RO; import static com.android.aconfig.test.Flags.FLAG_ENABLED_RO; import static com.android.aconfig.test.Flags.FLAG_ENABLED_RW; import static com.android.aconfig.test.Flags.disabledRo; import static com.android.aconfig.test.Flags.disabledRw; +import static com.android.aconfig.test.Flags.enabledFixedRo; import static com.android.aconfig.test.Flags.enabledRo; import static com.android.aconfig.test.Flags.enabledRw; import static org.junit.Assert.assertEquals; @@ -34,6 +36,14 @@ public final class AconfigTest { assertFalse(enabledRo()); } + @Test + public void testEnabledFixedReadOnlyFlag() { + assertEquals("com.android.aconfig.test.enabled_fixed_ro", FLAG_ENABLED_FIXED_RO); + // TODO: change to assertTrue(enabledFixedRo()) when the build supports reading tests/*.values + // (currently all flags are assigned the default READ_ONLY + DISABLED) + assertFalse(enabledFixedRo()); + } + @Test public void testDisabledReadWriteFlag() { assertEquals("com.android.aconfig.test.enabled_ro", FLAG_ENABLED_RO); diff --git a/tools/aconfig/tests/first.values b/tools/aconfig/tests/first.values index e5244041a3..a450f7869e 100644 --- a/tools/aconfig/tests/first.values +++ b/tools/aconfig/tests/first.values @@ -16,3 +16,9 @@ flag_value { state: ENABLED permission: READ_WRITE } +flag_value { + package: "com.android.aconfig.test" + name: "enabled_fixed_ro" + state: ENABLED + permission: READ_ONLY +} diff --git a/tools/aconfig/tests/test.aconfig b/tools/aconfig/tests/test.aconfig index 46cf1e9a76..aaa6df585e 100644 --- a/tools/aconfig/tests/test.aconfig +++ b/tools/aconfig/tests/test.aconfig @@ -40,3 +40,14 @@ flag { description: "This flag is DISABLED + READ_WRITE" bug: "456" } + +# This flag's final value calculated from: +# - test.aconfig: DISABLED + READ_ONLY +# - first.values: ENABLED + READ_ONLY +flag { + name: "enabled_fixed_ro" + namespace: "aconfig_test" + description: "This flag is fixed READ_ONLY + ENABLED" + bug: "" + is_fixed_read_only: true +}