From 53359553176c12ee41c3973ea468e44d31aaf983 Mon Sep 17 00:00:00 2001 From: Zhuoyao Zhang Date: Mon, 16 Sep 2024 23:58:11 +0000 Subject: [PATCH 1/4] The initial implementation of the edit monitor Creates a class that will manager and monitor the actual edit watchdog that run as a subprocess. As a first step, the class supports: 1) A start method that creates a pidfile and starts a subprocess with given target/args. 2) A stop method that terminates the created subprocess and removes the pidfile. Detailed design in go/android-local-edit-monitor. Test: atest daemon_manager_test bug: 365617369 Change-Id: Ic6d7be67d284ade8033416235b9b0fb1e90e1b1a --- tools/edit_monitor/Android.bp | 23 +++ tools/edit_monitor/daemon_manager.py | 165 ++++++++++++++++++ tools/edit_monitor/daemon_manager_test.py | 197 ++++++++++++++++++++++ 3 files changed, 385 insertions(+) create mode 100644 tools/edit_monitor/daemon_manager.py create mode 100644 tools/edit_monitor/daemon_manager_test.py diff --git a/tools/edit_monitor/Android.bp b/tools/edit_monitor/Android.bp index 80437c00d4..b939633817 100644 --- a/tools/edit_monitor/Android.bp +++ b/tools/edit_monitor/Android.bp @@ -19,3 +19,26 @@ package { default_applicable_licenses: ["Android-Apache-2.0"], default_team: "trendy_team_adte", } + +python_library_host { + name: "edit_monitor_lib", + pkg_path: "edit_monitor", + srcs: [ + "daemon_manager.py", + ], +} + +python_test_host { + name: "daemon_manager_test", + main: "daemon_manager_test.py", + pkg_path: "edit_monitor", + srcs: [ + "daemon_manager_test.py", + ], + libs: [ + "edit_monitor_lib", + ], + test_options: { + unit_test: true, + }, +} diff --git a/tools/edit_monitor/daemon_manager.py b/tools/edit_monitor/daemon_manager.py new file mode 100644 index 0000000000..c09c3213ae --- /dev/null +++ b/tools/edit_monitor/daemon_manager.py @@ -0,0 +1,165 @@ +# Copyright 2024, The Android Open Source Project +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + + +import hashlib +import logging +import multiprocessing +import os +import pathlib +import signal +import subprocess +import tempfile +import time + + +DEFAULT_PROCESS_TERMINATION_TIMEOUT_SECONDS = 1 + + +def default_daemon_target(): + """Place holder for the default daemon target.""" + print("default daemon target") + + +class DaemonManager: + """Class to manage and monitor the daemon run as a subprocess.""" + + def __init__( + self, + binary_path: str, + daemon_target: callable = default_daemon_target, + daemon_args: tuple = (), + ): + self.binary_path = binary_path + self.daemon_target = daemon_target + self.daemon_args = daemon_args + + self.pid = os.getpid() + self.daemon_process = None + + pid_file_dir = pathlib.Path(tempfile.gettempdir()).joinpath("edit_monitor") + pid_file_dir.mkdir(parents=True, exist_ok=True) + self.pid_file_path = self._get_pid_file_path(pid_file_dir) + + def start(self): + """Writes the pidfile and starts the daemon proces.""" + try: + self._write_pid_to_pidfile() + self._start_daemon_process() + except Exception as e: + logging.exception("Failed to start daemon manager with error %s", e) + + def stop(self): + """Stops the daemon process and removes the pidfile.""" + + logging.debug("in daemon manager cleanup.") + try: + if self.daemon_process and self.daemon_process.is_alive(): + self._terminate_process(self.daemon_process.pid) + self._remove_pidfile() + except Exception as e: + logging.exception("Failed to stop daemon manager with error %s", e) + + def _write_pid_to_pidfile(self): + """Creates a pidfile and writes the current pid to the file. + + Raise FileExistsError if the pidfile already exists. + """ + try: + # Use the 'x' mode to open the file for exclusive creation + with open(self.pid_file_path, "x") as f: + f.write(f"{self.pid}") + except FileExistsError as e: + # This could be caused due to race condition that a user is trying + # to start two edit monitors at the same time. Or because there is + # already an existing edit monitor running and we can not kill it + # for some reason. + logging.exception("pidfile %s already exists.", self.pid_file_path) + raise e + + def _start_daemon_process(self): + """Starts a subprocess to run the daemon.""" + p = multiprocessing.Process( + target=self.daemon_target, args=self.daemon_args + ) + p.start() + + logging.info("Start subprocess with PID %d", p.pid) + self.daemon_process = p + + def _terminate_process( + self, pid: int, timeout: int = DEFAULT_PROCESS_TERMINATION_TIMEOUT_SECONDS + ): + """Terminates a process with given pid. + + It first sends a SIGTERM to the process to allow it for proper + termination with a timeout. If the process is not terminated within + the timeout, kills it forcefully. + """ + try: + os.kill(pid, signal.SIGTERM) + if not self._wait_for_process_terminate(pid, timeout): + logging.warning( + "Process %d not terminated within timeout, try force kill", pid + ) + os.kill(pid, signal.SIGKILL) + except ProcessLookupError: + logging.info("Process with PID %d not found (already terminated)", pid) + + def _wait_for_process_terminate(self, pid: int, timeout: int) -> bool: + start_time = time.time() + + while time.time() < start_time + timeout: + if not self._is_process_alive(pid): + return True + time.sleep(1) + + logging.error("Process %d not terminated within %d seconds.", pid, timeout) + return False + + def _is_process_alive(self, pid: int) -> bool: + try: + output = subprocess.check_output( + ["ps", "-p", str(pid), "-o", "state="], text=True + ).strip() + state = output.split()[0] + return state != "Z" # Check if the state is not 'Z' (zombie) + except subprocess.CalledProcessError: + # Process not found (already dead). + return False + except (FileNotFoundError, OSError, ValueError) as e: + logging.warning( + "Unable to check the status for process %d with error: %s.", pid, e + ) + return True + + def _remove_pidfile(self): + try: + os.remove(self.pid_file_path) + except FileNotFoundError: + logging.info("pid file %s already removed.", self.pid_file_path) + + def _get_pid_file_path(self, pid_file_dir: pathlib.Path) -> pathlib.Path: + """Generates the path to store the pidfile. + + The file path should have the format of "/tmp/edit_monitor/xxxx.lock" + where xxxx is a hashed value based on the binary path that starts the + process. + """ + hash_object = hashlib.sha256() + hash_object.update(self.binary_path.encode("utf-8")) + pid_file_path = pid_file_dir.joinpath(hash_object.hexdigest() + ".lock") + logging.info("pid_file_path: %s", pid_file_path) + + return pid_file_path diff --git a/tools/edit_monitor/daemon_manager_test.py b/tools/edit_monitor/daemon_manager_test.py new file mode 100644 index 0000000000..5be4beee51 --- /dev/null +++ b/tools/edit_monitor/daemon_manager_test.py @@ -0,0 +1,197 @@ +# Copyright 2024, The Android Open Source Project +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Unittests for DaemonManager.""" + +import logging +import multiprocessing +import os +import pathlib +import signal +import subprocess +import sys +import tempfile +import time +import unittest +from unittest import mock +from edit_monitor import daemon_manager + +TEST_BINARY_FILE = '/path/to/test_binary' +TEST_PID_FILE_PATH = ( + '587239c2d1050afdf54512e2d799f3b929f86b43575eb3c7b4bab105dd9bd25e.lock' +) + + +def example_daemon(output_file): + with open(output_file, 'w') as f: + f.write('running daemon target') + + +def long_running_daemon(): + while True: + time.sleep(1) + + +class DaemonManagerTest(unittest.TestCase): + + @classmethod + def setUpClass(cls): + super().setUpClass() + # Configure to print logging to stdout. + logging.basicConfig(filename=None, level=logging.DEBUG) + console = logging.StreamHandler(sys.stdout) + logging.getLogger('').addHandler(console) + + def setUp(self): + super().setUp() + self.original_tempdir = tempfile.tempdir + self.working_dir = tempfile.TemporaryDirectory() + # Sets the tempdir under the working dir so any temp files created during + # tests will be cleaned. + tempfile.tempdir = self.working_dir.name + + def tearDown(self): + # Cleans up any child processes left by the tests. + self._cleanup_child_processes() + self.working_dir.cleanup() + # Restores tempdir. + tempfile.tempdir = self.original_tempdir + super().tearDown() + + def test_start_success(self): + damone_output_file = tempfile.NamedTemporaryFile( + dir=self.working_dir.name, delete=False + ) + dm = daemon_manager.DaemonManager( + TEST_BINARY_FILE, + daemon_target=example_daemon, + daemon_args=(damone_output_file.name,), + ) + dm.start() + dm.daemon_process.join() + + # Verifies the expected pid file is created. + expected_pid_file_path = pathlib.Path(self.working_dir.name).joinpath( + 'edit_monitor', TEST_PID_FILE_PATH + ) + self.assertEqual(dm.pid_file_path, expected_pid_file_path) + self.assertTrue(expected_pid_file_path.exists()) + + # Verifies the daemon process is executed successfully. + with open(damone_output_file.name, 'r') as f: + contents = f.read() + self.assertEqual(contents, 'running daemon target') + + def test_start_failed_to_write_pidfile(self): + pid_file_path_dir = pathlib.Path(self.working_dir.name).joinpath( + 'edit_monitor' + ) + pid_file_path_dir.mkdir(parents=True, exist_ok=True) + # Makes the directory read-only so write pidfile will fail. + os.chmod(pid_file_path_dir, 0o555) + + dm = daemon_manager.DaemonManager(TEST_BINARY_FILE) + dm.start() + + # Verifies no daemon process is started. + self.assertIsNone(dm.daemon_process) + + def test_start_failed_to_start_daemon_process(self): + dm = daemon_manager.DaemonManager( + TEST_BINARY_FILE, daemon_target='wrong_target', daemon_args=(1) + ) + dm.start() + + # Verifies no daemon process is started. + self.assertIsNone(dm.daemon_process) + + def test_stop_success(self): + dm = daemon_manager.DaemonManager( + TEST_BINARY_FILE, daemon_target=long_running_daemon + ) + dm.start() + dm.stop() + + self.assert_no_subprocess_running() + self.assertFalse(dm.pid_file_path.exists()) + + @mock.patch('os.kill') + def test_stop_failed_to_kill_daemon_process(self, mock_kill): + mock_kill.side_effect = OSError('Unknown OSError') + dm = daemon_manager.DaemonManager( + TEST_BINARY_FILE, daemon_target=long_running_daemon + ) + dm.start() + dm.stop() + + self.assertTrue(dm.daemon_process.is_alive()) + self.assertTrue(dm.pid_file_path.exists()) + + @mock.patch('os.remove') + def test_stop_failed_to_remove_pidfile(self, mock_remove): + mock_remove.side_effect = OSError('Unknown OSError') + + dm = daemon_manager.DaemonManager( + TEST_BINARY_FILE, daemon_target=long_running_daemon + ) + dm.start() + dm.stop() + + self.assert_no_subprocess_running() + self.assertTrue(dm.pid_file_path.exists()) + + def assert_no_subprocess_running(self): + child_pids = self._get_child_processes(os.getpid()) + for child_pid in child_pids: + self.assertFalse( + self._is_process_alive(child_pid), f'process {child_pid} still alive' + ) + + def _get_child_processes(self, parent_pid): + try: + output = subprocess.check_output( + ['ps', '-o', 'pid,ppid', '--no-headers'], text=True + ) + + child_processes = [] + for line in output.splitlines(): + pid, ppid = line.split() + if int(ppid) == parent_pid: + child_processes.append(int(pid)) + return child_processes + except subprocess.CalledProcessError as e: + self.fail(f'failed to get child process, error: {e}') + + def _is_process_alive(self, pid): + try: + output = subprocess.check_output( + ['ps', '-p', str(pid), '-o', 'state='], text=True + ).strip() + state = output.split()[0] + return state != 'Z' # Check if the state is not 'Z' (zombie) + except subprocess.CalledProcessError: + return False + + def _cleanup_child_processes(self): + child_pids = self._get_child_processes(os.getpid()) + for child_pid in child_pids: + try: + os.kill(child_pid, signal.SIGKILL) + except ProcessLookupError: + # process already terminated + pass + + +if __name__ == '__main__': + unittest.main() From a80e298d34a3cfcaf1f89bc9c808c8b175095c4f Mon Sep 17 00:00:00 2001 From: Marybeth Fair Date: Mon, 23 Sep 2024 19:37:21 +0000 Subject: [PATCH 2/4] Revert "Add fingerprint to packages.map." This reverts commit 769d8eed378d468be6c8d840343772510752e4f1. Reason for revert: Caused crashed in places where old version files were present. Change-Id: I7d529773226cd834979400aa018c47bbf6891b72 --- tools/aconfig/aconfig/Android.bp | 8 -- tools/aconfig/aconfig/src/commands.rs | 74 +++++------------- tools/aconfig/aconfig/src/storage/mod.rs | 9 +-- .../aconfig/src/storage/package_table.rs | 1 - tools/aconfig/aconfig/src/test.rs | 18 ----- tools/aconfig/aconfig/tests/test.aconfig | 2 +- .../aconfig/tests/test_second_package.aconfig | 12 --- tools/aconfig/aconfig/tests/third.values | 6 -- .../aconfig_storage_file/src/flag_info.rs | 2 +- .../aconfig_storage_file/src/flag_table.rs | 2 +- .../aconfig_storage_file/src/flag_value.rs | 2 +- tools/aconfig/aconfig_storage_file/src/lib.rs | 12 +-- .../aconfig_storage_file/src/package_table.rs | 18 +---- .../aconfig_storage_file/src/test_utils.rs | 17 ++-- .../aconfig/storage/ByteBufferReader.java | 4 - .../android/aconfig/storage/PackageTable.java | 7 -- .../aconfig_storage_file/tests/flag.info | Bin 35 -> 35 bytes .../aconfig_storage_file/tests/flag.map | Bin 321 -> 321 bytes .../aconfig_storage_file/tests/flag.val | Bin 35 -> 35 bytes .../aconfig_storage_file/tests/package.map | Bin 233 -> 209 bytes .../tests/srcs/FlagTableTest.java | 2 +- .../tests/srcs/FlagValueListTest.java | 2 +- .../tests/srcs/PackageTableTest.java | 59 +++++++------- .../aconfig_storage_read_api/src/lib.rs | 8 +- .../aconfig_storage_read_api/tests/flag.info | Bin 35 -> 35 bytes .../aconfig_storage_read_api/tests/flag.map | Bin 321 -> 321 bytes .../aconfig_storage_read_api/tests/flag.val | Bin 35 -> 35 bytes .../tests/package.map | Bin 233 -> 209 bytes .../tests/storage_read_api_test.cpp | 8 +- .../tests/storage_read_api_test.rs | 8 +- 30 files changed, 80 insertions(+), 201 deletions(-) delete mode 100644 tools/aconfig/aconfig/tests/test_second_package.aconfig delete mode 100644 tools/aconfig/aconfig/tests/third.values diff --git a/tools/aconfig/aconfig/Android.bp b/tools/aconfig/aconfig/Android.bp index 5037783fb5..68521af91f 100644 --- a/tools/aconfig/aconfig/Android.bp +++ b/tools/aconfig/aconfig/Android.bp @@ -68,14 +68,6 @@ aconfig_values { ], } -aconfig_values { - name: "aconfig.test.flag.second_values", - package: "com.android.aconfig.test", - srcs: [ - "tests/third.values", - ], -} - aconfig_value_set { name: "aconfig.test.flag.value_set", values: [ diff --git a/tools/aconfig/aconfig/src/commands.rs b/tools/aconfig/aconfig/src/commands.rs index b5854165bc..797a893ff1 100644 --- a/tools/aconfig/aconfig/src/commands.rs +++ b/tools/aconfig/aconfig/src/commands.rs @@ -17,7 +17,7 @@ use anyhow::{bail, ensure, Context, Result}; use itertools::Itertools; use protobuf::Message; -use std::collections::HashMap; +use std::collections::{BTreeMap, HashMap}; use std::hash::Hasher; use std::io::Read; use std::path::PathBuf; @@ -422,30 +422,23 @@ where Ok(flag_ids) } -// Creates a fingerprint of the flag names. Sorts the vector. -pub fn compute_flags_fingerprint(flag_names: &mut Vec) -> Result { - flag_names.sort(); - - let mut hasher = SipHasher13::new(); - for flag in flag_names { - hasher.write(flag.as_bytes()); - } - Ok(hasher.finish()) -} - #[allow(dead_code)] // TODO: b/316357686 - Use fingerprint in codegen to // protect hardcoded offset reads. -fn compute_fingerprint_from_parsed_flags(flags: ProtoParsedFlags) -> Result { - let separated_flags: Vec = flags.parsed_flag.into_iter().collect::>(); +pub fn compute_flag_offsets_fingerprint(flags_map: &HashMap) -> Result { + let mut hasher = SipHasher13::new(); - // All flags must belong to the same package as the fingerprint is per-package. - let Some(_package) = find_unique_package(&separated_flags) else { - bail!("No parsed flags, or the parsed flags use different packages."); - }; + // Need to sort to ensure the data is added to the hasher in the same order + // each run. + let sorted_map: BTreeMap<&String, &u16> = flags_map.iter().collect(); - let mut flag_names = - separated_flags.into_iter().map(|flag| flag.name.unwrap()).collect::>(); - compute_flags_fingerprint(&mut flag_names) + for (flag, offset) in sorted_map { + // See https://docs.rs/siphasher/latest/siphasher/#note for use of write + // over write_i16. Similarly, use to_be_bytes rather than to_ne_bytes to + // ensure consistency. + hasher.write(flag.as_bytes()); + hasher.write(&offset.to_be_bytes()); + } + Ok(hasher.finish()) } #[cfg(test)] @@ -456,46 +449,15 @@ mod tests { #[test] fn test_offset_fingerprint() { let parsed_flags = crate::test::parse_test_flags(); - let expected_fingerprint: u64 = 5801144784618221668; + let package = find_unique_package(&parsed_flags.parsed_flag).unwrap().to_string(); + let flag_ids = assign_flag_ids(&package, parsed_flags.parsed_flag.iter()).unwrap(); + let expected_fingerprint = 10709892481002252132u64; - let hash_result = compute_fingerprint_from_parsed_flags(parsed_flags); + let hash_result = compute_flag_offsets_fingerprint(&flag_ids); assert_eq!(hash_result.unwrap(), expected_fingerprint); } - #[test] - fn test_offset_fingerprint_matches_from_package() { - let parsed_flags: ProtoParsedFlags = crate::test::parse_test_flags(); - - // All test flags are in the same package, so fingerprint from all of them. - let result_from_parsed_flags = compute_fingerprint_from_parsed_flags(parsed_flags.clone()); - - let mut flag_names_vec = parsed_flags - .parsed_flag - .clone() - .into_iter() - .map(|flag| flag.name.unwrap()) - .map(String::from) - .collect::>(); - let result_from_names = compute_flags_fingerprint(&mut flag_names_vec); - - // Assert the same hash is generated for each case. - assert_eq!(result_from_parsed_flags.unwrap(), result_from_names.unwrap()); - } - - #[test] - fn test_offset_fingerprint_different_packages_does_not_match() { - // Parse flags from two packages. - let parsed_flags: ProtoParsedFlags = crate::test::parse_test_flags(); - let second_parsed_flags = crate::test::parse_second_package_flags(); - - let result_from_parsed_flags = compute_fingerprint_from_parsed_flags(parsed_flags).unwrap(); - let second_result = compute_fingerprint_from_parsed_flags(second_parsed_flags).unwrap(); - - // Different flags should have a different fingerprint. - assert_ne!(result_from_parsed_flags, second_result); - } - #[test] fn test_parse_flags() { let parsed_flags = crate::test::parse_test_flags(); // calls parse_flags diff --git a/tools/aconfig/aconfig/src/storage/mod.rs b/tools/aconfig/aconfig/src/storage/mod.rs index efce24e422..73339f24b3 100644 --- a/tools/aconfig/aconfig/src/storage/mod.rs +++ b/tools/aconfig/aconfig/src/storage/mod.rs @@ -25,14 +25,12 @@ use crate::storage::{ flag_table::create_flag_table, flag_value::create_flag_value, package_table::create_package_table, }; -use aconfig_protos::ProtoParsedFlag; -use aconfig_protos::ProtoParsedFlags; +use aconfig_protos::{ProtoParsedFlag, ProtoParsedFlags}; use aconfig_storage_file::StorageFileType; pub struct FlagPackage<'a> { pub package_name: &'a str, pub package_id: u32, - pub fingerprint: u64, pub flag_names: HashSet<&'a str>, pub boolean_flags: Vec<&'a ProtoParsedFlag>, // The index of the first boolean flag in this aconfig package among all boolean @@ -45,7 +43,6 @@ impl<'a> FlagPackage<'a> { FlagPackage { package_name, package_id, - fingerprint: 0, flag_names: HashSet::new(), boolean_flags: vec![], boolean_start_index: 0, @@ -81,8 +78,6 @@ where for p in packages.iter_mut() { p.boolean_start_index = boolean_start_index; boolean_start_index += p.boolean_flags.len() as u32; - - // TODO: b/316357686 - Calculate fingerprint and add to package. } packages @@ -120,8 +115,6 @@ mod tests { use super::*; use crate::Input; - use aconfig_protos::ProtoParsedFlags; - pub fn parse_all_test_flags() -> Vec { let aconfig_files = [ ( diff --git a/tools/aconfig/aconfig/src/storage/package_table.rs b/tools/aconfig/aconfig/src/storage/package_table.rs index 33bb0774e0..c53602f9cb 100644 --- a/tools/aconfig/aconfig/src/storage/package_table.rs +++ b/tools/aconfig/aconfig/src/storage/package_table.rs @@ -48,7 +48,6 @@ impl PackageTableNodeWrapper { let node = PackageTableNode { package_name: String::from(package.package_name), package_id: package.package_id, - fingerprint: package.fingerprint, boolean_start_index: package.boolean_start_index, next_offset: None, }; diff --git a/tools/aconfig/aconfig/src/test.rs b/tools/aconfig/aconfig/src/test.rs index a19b372aac..7409cda6e8 100644 --- a/tools/aconfig/aconfig/src/test.rs +++ b/tools/aconfig/aconfig/src/test.rs @@ -295,24 +295,6 @@ parsed_flag { aconfig_protos::parsed_flags::try_from_binary_proto(&bytes).unwrap() } - pub fn parse_second_package_flags() -> ProtoParsedFlags { - let bytes = crate::commands::parse_flags( - "com.android.aconfig.second_test", - Some("system"), - vec![Input { - source: "tests/test_second_package.aconfig".to_string(), - reader: Box::new(include_bytes!("../tests/test_second_package.aconfig").as_slice()), - }], - vec![Input { - source: "tests/third.values".to_string(), - reader: Box::new(include_bytes!("../tests/third.values").as_slice()), - }], - crate::commands::DEFAULT_FLAG_PERMISSION, - ) - .unwrap(); - aconfig_protos::parsed_flags::try_from_binary_proto(&bytes).unwrap() - } - pub fn first_significant_code_diff(a: &str, b: &str) -> Option { let a = a.lines().map(|line| line.trim_start()).filter(|line| !line.is_empty()); let b = b.lines().map(|line| line.trim_start()).filter(|line| !line.is_empty()); diff --git a/tools/aconfig/aconfig/tests/test.aconfig b/tools/aconfig/aconfig/tests/test.aconfig index a818b2332e..c11508aabc 100644 --- a/tools/aconfig/aconfig/tests/test.aconfig +++ b/tools/aconfig/aconfig/tests/test.aconfig @@ -86,4 +86,4 @@ flag { bug: "111" is_fixed_read_only: true is_exported: true -} +} \ No newline at end of file diff --git a/tools/aconfig/aconfig/tests/test_second_package.aconfig b/tools/aconfig/aconfig/tests/test_second_package.aconfig deleted file mode 100644 index a8740b85dd..0000000000 --- a/tools/aconfig/aconfig/tests/test_second_package.aconfig +++ /dev/null @@ -1,12 +0,0 @@ -package: "com.android.aconfig.second_test" -container: "system" - -flag { - name: "testing_flag" - namespace: "another_namespace" - description: "This is a flag for testing." - bug: "123" - metadata { - purpose: PURPOSE_UNSPECIFIED - } -} diff --git a/tools/aconfig/aconfig/tests/third.values b/tools/aconfig/aconfig/tests/third.values deleted file mode 100644 index 675832a4bc..0000000000 --- a/tools/aconfig/aconfig/tests/third.values +++ /dev/null @@ -1,6 +0,0 @@ -flag_value { - package: "com.android.aconfig.second_test" - name: "testing_flag" - state: DISABLED - permission: READ_WRITE -} diff --git a/tools/aconfig/aconfig_storage_file/src/flag_info.rs b/tools/aconfig/aconfig_storage_file/src/flag_info.rs index a49756d012..f090396901 100644 --- a/tools/aconfig/aconfig_storage_file/src/flag_info.rs +++ b/tools/aconfig/aconfig_storage_file/src/flag_info.rs @@ -227,7 +227,7 @@ mod tests { let bytes = &flag_info_list.into_bytes(); let mut head = 0; let version = read_u32_from_bytes(bytes, &mut head).unwrap(); - assert_eq!(version, 2); + assert_eq!(version, 1); } #[test] diff --git a/tools/aconfig/aconfig_storage_file/src/flag_table.rs b/tools/aconfig/aconfig_storage_file/src/flag_table.rs index be82c63028..0588fe5039 100644 --- a/tools/aconfig/aconfig_storage_file/src/flag_table.rs +++ b/tools/aconfig/aconfig_storage_file/src/flag_table.rs @@ -253,7 +253,7 @@ mod tests { let bytes = &flag_table.into_bytes(); let mut head = 0; let version = read_u32_from_bytes(bytes, &mut head).unwrap(); - assert_eq!(version, 2); + assert_eq!(version, 1); } #[test] diff --git a/tools/aconfig/aconfig_storage_file/src/flag_value.rs b/tools/aconfig/aconfig_storage_file/src/flag_value.rs index c4cf29451e..b64c10ecdd 100644 --- a/tools/aconfig/aconfig_storage_file/src/flag_value.rs +++ b/tools/aconfig/aconfig_storage_file/src/flag_value.rs @@ -159,7 +159,7 @@ mod tests { let bytes = &flag_value_list.into_bytes(); let mut head = 0; let version = read_u32_from_bytes(bytes, &mut head).unwrap(); - assert_eq!(version, 2); + assert_eq!(version, 1); } #[test] diff --git a/tools/aconfig/aconfig_storage_file/src/lib.rs b/tools/aconfig/aconfig_storage_file/src/lib.rs index 19d0e51e61..cf52bc017d 100644 --- a/tools/aconfig/aconfig_storage_file/src/lib.rs +++ b/tools/aconfig/aconfig_storage_file/src/lib.rs @@ -58,7 +58,7 @@ use crate::AconfigStorageError::{ }; /// Storage file version -pub const FILE_VERSION: u32 = 2; +pub const FILE_VERSION: u32 = 1; /// Good hash table prime number pub(crate) const HASH_PRIMES: [u32; 29] = [ @@ -254,16 +254,6 @@ pub fn read_u32_from_bytes(buf: &[u8], head: &mut usize) -> Result Result { - let val = - u64::from_le_bytes(buf[*head..*head + 8].try_into().map_err(|errmsg| { - BytesParseFail(anyhow!("fail to parse u64 from bytes: {}", errmsg)) - })?); - *head += 8; - Ok(val) -} - /// Read and parse bytes as string pub(crate) fn read_str_from_bytes( buf: &[u8], diff --git a/tools/aconfig/aconfig_storage_file/src/package_table.rs b/tools/aconfig/aconfig_storage_file/src/package_table.rs index 350f072df2..a5bd9e6446 100644 --- a/tools/aconfig/aconfig_storage_file/src/package_table.rs +++ b/tools/aconfig/aconfig_storage_file/src/package_table.rs @@ -17,10 +17,7 @@ //! package table module defines the package table file format and methods for serialization //! and deserialization -use crate::{ - get_bucket_index, read_str_from_bytes, read_u32_from_bytes, read_u64_from_bytes, - read_u8_from_bytes, -}; +use crate::{get_bucket_index, read_str_from_bytes, read_u32_from_bytes, read_u8_from_bytes}; use crate::{AconfigStorageError, StorageFileType}; use anyhow::anyhow; use serde::{Deserialize, Serialize}; @@ -100,7 +97,6 @@ impl PackageTableHeader { pub struct PackageTableNode { pub package_name: String, pub package_id: u32, - pub fingerprint: u64, // The index of the first boolean flag in this aconfig package among all boolean // flags in this container. pub boolean_start_index: u32, @@ -112,12 +108,8 @@ impl fmt::Debug for PackageTableNode { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { writeln!( f, - "Package: {}, Id: {}, Fingerprint: {}, Boolean flag start index: {}, Next: {:?}", - self.package_name, - self.package_id, - self.fingerprint, - self.boolean_start_index, - self.next_offset + "Package: {}, Id: {}, Boolean flag start index: {}, Next: {:?}", + self.package_name, self.package_id, self.boolean_start_index, self.next_offset )?; Ok(()) } @@ -131,7 +123,6 @@ impl PackageTableNode { result.extend_from_slice(&(name_bytes.len() as u32).to_le_bytes()); result.extend_from_slice(name_bytes); result.extend_from_slice(&self.package_id.to_le_bytes()); - result.extend_from_slice(&self.fingerprint.to_le_bytes()); result.extend_from_slice(&self.boolean_start_index.to_le_bytes()); result.extend_from_slice(&self.next_offset.unwrap_or(0).to_le_bytes()); result @@ -143,7 +134,6 @@ impl PackageTableNode { let node = Self { package_name: read_str_from_bytes(bytes, &mut head)?, package_id: read_u32_from_bytes(bytes, &mut head)?, - fingerprint: read_u64_from_bytes(bytes, &mut head)?, boolean_start_index: read_u32_from_bytes(bytes, &mut head)?, next_offset: match read_u32_from_bytes(bytes, &mut head)? { 0 => None, @@ -261,7 +251,7 @@ mod tests { let bytes = &package_table.into_bytes(); let mut head = 0; let version = read_u32_from_bytes(bytes, &mut head).unwrap(); - assert_eq!(version, 2); + assert_eq!(version, 1); } #[test] diff --git a/tools/aconfig/aconfig_storage_file/src/test_utils.rs b/tools/aconfig/aconfig_storage_file/src/test_utils.rs index 11e2dc688e..106666c47f 100644 --- a/tools/aconfig/aconfig_storage_file/src/test_utils.rs +++ b/tools/aconfig/aconfig_storage_file/src/test_utils.rs @@ -26,33 +26,30 @@ use tempfile::NamedTempFile; pub fn create_test_package_table() -> PackageTable { let header = PackageTableHeader { - version: 2, + version: 1, container: String::from("mockup"), file_type: StorageFileType::PackageMap as u8, - file_size: 233, + file_size: 209, num_packages: 3, bucket_offset: 31, node_offset: 59, }; - let buckets: Vec> = vec![Some(59), None, None, Some(117), None, None, None]; + let buckets: Vec> = vec![Some(59), None, None, Some(109), None, None, None]; let first_node = PackageTableNode { package_name: String::from("com.android.aconfig.storage.test_2"), package_id: 1, - fingerprint: 0, boolean_start_index: 3, next_offset: None, }; let second_node = PackageTableNode { package_name: String::from("com.android.aconfig.storage.test_1"), package_id: 0, - fingerprint: 0, boolean_start_index: 0, - next_offset: Some(175), + next_offset: Some(159), }; let third_node = PackageTableNode { package_name: String::from("com.android.aconfig.storage.test_4"), package_id: 2, - fingerprint: 0, boolean_start_index: 6, next_offset: None, }; @@ -81,7 +78,7 @@ impl FlagTableNode { pub fn create_test_flag_table() -> FlagTable { let header = FlagTableHeader { - version: 2, + version: 1, container: String::from("mockup"), file_type: StorageFileType::FlagMap as u8, file_size: 321, @@ -123,7 +120,7 @@ pub fn create_test_flag_table() -> FlagTable { pub fn create_test_flag_value_list() -> FlagValueList { let header = FlagValueHeader { - version: 2, + version: 1, container: String::from("mockup"), file_type: StorageFileType::FlagVal as u8, file_size: 35, @@ -136,7 +133,7 @@ pub fn create_test_flag_value_list() -> FlagValueList { pub fn create_test_flag_info_list() -> FlagInfoList { let header = FlagInfoHeader { - version: 2, + version: 1, container: String::from("mockup"), file_type: StorageFileType::FlagInfo as u8, file_size: 35, diff --git a/tools/aconfig/aconfig_storage_file/srcs/android/aconfig/storage/ByteBufferReader.java b/tools/aconfig/aconfig_storage_file/srcs/android/aconfig/storage/ByteBufferReader.java index 44a82ee1c7..4bea0836f0 100644 --- a/tools/aconfig/aconfig_storage_file/srcs/android/aconfig/storage/ByteBufferReader.java +++ b/tools/aconfig/aconfig_storage_file/srcs/android/aconfig/storage/ByteBufferReader.java @@ -37,10 +37,6 @@ public class ByteBufferReader { return Short.toUnsignedInt(mByteBuffer.getShort()); } - public long readLong() { - return mByteBuffer.getLong(); - } - public int readInt() { return this.mByteBuffer.getInt(); } diff --git a/tools/aconfig/aconfig_storage_file/srcs/android/aconfig/storage/PackageTable.java b/tools/aconfig/aconfig_storage_file/srcs/android/aconfig/storage/PackageTable.java index f1288f5ea6..773b882f4a 100644 --- a/tools/aconfig/aconfig_storage_file/srcs/android/aconfig/storage/PackageTable.java +++ b/tools/aconfig/aconfig_storage_file/srcs/android/aconfig/storage/PackageTable.java @@ -118,7 +118,6 @@ public class PackageTable { private String mPackageName; private int mPackageId; - private long mFingerprint; private int mBooleanStartIndex; private int mNextOffset; @@ -126,7 +125,6 @@ public class PackageTable { Node node = new Node(); node.mPackageName = reader.readString(); node.mPackageId = reader.readInt(); - node.mFingerprint = reader.readLong(); node.mBooleanStartIndex = reader.readInt(); node.mNextOffset = reader.readInt(); node.mNextOffset = node.mNextOffset == 0 ? -1 : node.mNextOffset; @@ -152,7 +150,6 @@ public class PackageTable { return Objects.equals(mPackageName, other.mPackageName) && mPackageId == other.mPackageId && mBooleanStartIndex == other.mBooleanStartIndex - && mFingerprint == other.mFingerprint && mNextOffset == other.mNextOffset; } @@ -168,10 +165,6 @@ public class PackageTable { return mBooleanStartIndex; } - public long getFingerprint() { - return mFingerprint; - } - public int getNextOffset() { return mNextOffset; } diff --git a/tools/aconfig/aconfig_storage_file/tests/flag.info b/tools/aconfig/aconfig_storage_file/tests/flag.info index 9db7fde7aedb036210391ed5e566611f25740aaf..6223edf369cbd0635aa34de23a676b1cf8aa3667 100644 GIT binary patch delta 7 OcmY#ZW@Mbms0085t^oZ2 delta 7 OcmY#ZW@MVks0085vH<=7 diff --git a/tools/aconfig/aconfig_storage_file/tests/flag.map b/tools/aconfig/aconfig_storage_file/tests/flag.map index cf4685ceb47f29c1f5268ceaf0247e5716127c67..e868f53d7e599f130604faac1fd20149e73cb821 100644 GIT binary patch delta 10 RcmX@ebdZUWaU-JxBLEQ(0(<}f delta 10 RcmX@ebdZUWX(OWpBLEQ;0(}4g diff --git a/tools/aconfig/aconfig_storage_file/tests/flag.val b/tools/aconfig/aconfig_storage_file/tests/flag.val index 37d4750206acbd5a3924f2aec4a75c75e4fe153f..ed203d4d13d068fc22d4136d413fe1fbd82cc57d 100644 GIT binary patch delta 7 OcmY#ZW@Mbms0085t^oZ2 delta 7 OcmY#ZW@MVks0085vH<=7 diff --git a/tools/aconfig/aconfig_storage_file/tests/package.map b/tools/aconfig/aconfig_storage_file/tests/package.map index 358010cf37cd8e82691b9f8c0e5d8d0f938671ab..6c46a0339cf94dde5e32e3e345835f8e893f6fba 100644 GIT binary patch delta 31 icmaFKc#)BjaU!D@W9~#9%Za&WjPob1(dA%cfB*oFKnE)T delta 70 ucmcb}_>z&4X(FQ*W9dX4OI`*jU=V%BS!bD*7`iXON1z;RDC<6d#F9)vx diff --git a/tools/aconfig/aconfig_storage_file/tests/srcs/FlagTableTest.java b/tools/aconfig/aconfig_storage_file/tests/srcs/FlagTableTest.java index e3b02cd666..fd40d4c4ef 100644 --- a/tools/aconfig/aconfig_storage_file/tests/srcs/FlagTableTest.java +++ b/tools/aconfig/aconfig_storage_file/tests/srcs/FlagTableTest.java @@ -33,7 +33,7 @@ public class FlagTableTest { public void testFlagTable_rightHeader() throws Exception { FlagTable flagTable = FlagTable.fromBytes(TestDataUtils.getTestFlagMapByteBuffer()); FlagTable.Header header = flagTable.getHeader(); - assertEquals(2, header.getVersion()); + assertEquals(1, header.getVersion()); assertEquals("mockup", header.getContainer()); assertEquals(FileType.FLAG_MAP, header.getFileType()); assertEquals(321, header.getFileSize()); diff --git a/tools/aconfig/aconfig_storage_file/tests/srcs/FlagValueListTest.java b/tools/aconfig/aconfig_storage_file/tests/srcs/FlagValueListTest.java index ebc231c2dd..1b0de630c7 100644 --- a/tools/aconfig/aconfig_storage_file/tests/srcs/FlagValueListTest.java +++ b/tools/aconfig/aconfig_storage_file/tests/srcs/FlagValueListTest.java @@ -36,7 +36,7 @@ public class FlagValueListTest { FlagValueList flagValueList = FlagValueList.fromBytes(TestDataUtils.getTestFlagValByteBuffer()); FlagValueList.Header header = flagValueList.getHeader(); - assertEquals(2, header.getVersion()); + assertEquals(1, header.getVersion()); assertEquals("mockup", header.getContainer()); assertEquals(FileType.FLAG_VAL, header.getFileType()); assertEquals(35, header.getFileSize()); diff --git a/tools/aconfig/aconfig_storage_file/tests/srcs/PackageTableTest.java b/tools/aconfig/aconfig_storage_file/tests/srcs/PackageTableTest.java index 6d56ceed96..e7e19d8d51 100644 --- a/tools/aconfig/aconfig_storage_file/tests/srcs/PackageTableTest.java +++ b/tools/aconfig/aconfig_storage_file/tests/srcs/PackageTableTest.java @@ -20,6 +20,7 @@ import static org.junit.Assert.assertEquals; import android.aconfig.storage.FileType; import android.aconfig.storage.PackageTable; + import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -27,40 +28,42 @@ import org.junit.runners.JUnit4; @RunWith(JUnit4.class) public class PackageTableTest { - @Test - public void testPackageTable_rightHeader() throws Exception { - PackageTable packageTable = PackageTable.fromBytes(TestDataUtils.getTestPackageMapByteBuffer()); - PackageTable.Header header = packageTable.getHeader(); - assertEquals(2, header.getVersion()); - assertEquals("mockup", header.getContainer()); - assertEquals(FileType.PACKAGE_MAP, header.getFileType()); - assertEquals(209, header.getFileSize()); - assertEquals(3, header.getNumPackages()); - assertEquals(31, header.getBucketOffset()); - assertEquals(59, header.getNodeOffset()); - } + @Test + public void testPackageTable_rightHeader() throws Exception { + PackageTable packageTable = + PackageTable.fromBytes(TestDataUtils.getTestPackageMapByteBuffer()); + PackageTable.Header header = packageTable.getHeader(); + assertEquals(1, header.getVersion()); + assertEquals("mockup", header.getContainer()); + assertEquals(FileType.PACKAGE_MAP, header.getFileType()); + assertEquals(209, header.getFileSize()); + assertEquals(3, header.getNumPackages()); + assertEquals(31, header.getBucketOffset()); + assertEquals(59, header.getNodeOffset()); + } - @Test - public void testPackageTable_rightNode() throws Exception { - PackageTable packageTable = PackageTable.fromBytes(TestDataUtils.getTestPackageMapByteBuffer()); + @Test + public void testPackageTable_rightNode() throws Exception { + PackageTable packageTable = + PackageTable.fromBytes(TestDataUtils.getTestPackageMapByteBuffer()); - PackageTable.Node node1 = packageTable.get("com.android.aconfig.storage.test_1"); - PackageTable.Node node2 = packageTable.get("com.android.aconfig.storage.test_2"); - PackageTable.Node node4 = packageTable.get("com.android.aconfig.storage.test_4"); + PackageTable.Node node1 = packageTable.get("com.android.aconfig.storage.test_1"); + PackageTable.Node node2 = packageTable.get("com.android.aconfig.storage.test_2"); + PackageTable.Node node4 = packageTable.get("com.android.aconfig.storage.test_4"); - assertEquals("com.android.aconfig.storage.test_1", node1.getPackageName()); - assertEquals("com.android.aconfig.storage.test_2", node2.getPackageName()); - assertEquals("com.android.aconfig.storage.test_4", node4.getPackageName()); + assertEquals("com.android.aconfig.storage.test_1", node1.getPackageName()); + assertEquals("com.android.aconfig.storage.test_2", node2.getPackageName()); + assertEquals("com.android.aconfig.storage.test_4", node4.getPackageName()); - assertEquals(0, node1.getPackageId()); - assertEquals(1, node2.getPackageId()); - assertEquals(2, node4.getPackageId()); + assertEquals(0, node1.getPackageId()); + assertEquals(1, node2.getPackageId()); + assertEquals(2, node4.getPackageId()); - assertEquals(0, node1.getBooleanStartIndex()); - assertEquals(3, node2.getBooleanStartIndex()); - assertEquals(6, node4.getBooleanStartIndex()); + assertEquals(0, node1.getBooleanStartIndex()); + assertEquals(3, node2.getBooleanStartIndex()); + assertEquals(6, node4.getBooleanStartIndex()); - assertEquals(175, node1.getNextOffset()); + assertEquals(159, node1.getNextOffset()); assertEquals(-1, node2.getNextOffset()); assertEquals(-1, node4.getNextOffset()); } diff --git a/tools/aconfig/aconfig_storage_read_api/src/lib.rs b/tools/aconfig/aconfig_storage_read_api/src/lib.rs index 59aa749c74..d76cf3fe4e 100644 --- a/tools/aconfig/aconfig_storage_read_api/src/lib.rs +++ b/tools/aconfig/aconfig_storage_read_api/src/lib.rs @@ -507,9 +507,9 @@ mod tests { #[test] // this test point locks down flag storage file version number query api fn test_storage_version_query() { - assert_eq!(get_storage_file_version("./tests/package.map").unwrap(), 2); - assert_eq!(get_storage_file_version("./tests/flag.map").unwrap(), 2); - assert_eq!(get_storage_file_version("./tests/flag.val").unwrap(), 2); - assert_eq!(get_storage_file_version("./tests/flag.info").unwrap(), 2); + assert_eq!(get_storage_file_version("./tests/package.map").unwrap(), 1); + assert_eq!(get_storage_file_version("./tests/flag.map").unwrap(), 1); + assert_eq!(get_storage_file_version("./tests/flag.val").unwrap(), 1); + assert_eq!(get_storage_file_version("./tests/flag.info").unwrap(), 1); } } diff --git a/tools/aconfig/aconfig_storage_read_api/tests/flag.info b/tools/aconfig/aconfig_storage_read_api/tests/flag.info index 9db7fde7aedb036210391ed5e566611f25740aaf..6223edf369cbd0635aa34de23a676b1cf8aa3667 100644 GIT binary patch delta 7 OcmY#ZW@Mbms0085t^oZ2 delta 7 OcmY#ZW@MVks0085vH<=7 diff --git a/tools/aconfig/aconfig_storage_read_api/tests/flag.map b/tools/aconfig/aconfig_storage_read_api/tests/flag.map index cf4685ceb47f29c1f5268ceaf0247e5716127c67..e868f53d7e599f130604faac1fd20149e73cb821 100644 GIT binary patch delta 10 RcmX@ebdZUWaU-JxBLEQ(0(<}f delta 10 RcmX@ebdZUWX(OWpBLEQ;0(}4g diff --git a/tools/aconfig/aconfig_storage_read_api/tests/flag.val b/tools/aconfig/aconfig_storage_read_api/tests/flag.val index 37d4750206acbd5a3924f2aec4a75c75e4fe153f..ed203d4d13d068fc22d4136d413fe1fbd82cc57d 100644 GIT binary patch delta 7 OcmY#ZW@Mbms0085t^oZ2 delta 7 OcmY#ZW@MVks0085vH<=7 diff --git a/tools/aconfig/aconfig_storage_read_api/tests/package.map b/tools/aconfig/aconfig_storage_read_api/tests/package.map index 358010cf37cd8e82691b9f8c0e5d8d0f938671ab..6c46a0339cf94dde5e32e3e345835f8e893f6fba 100644 GIT binary patch delta 31 icmaFKc#)BjaU!D@W9~#9%Za&WjPob1(dA%cfB*oFKnE)T delta 70 ucmcb}_>z&4X(FQ*W9dX4OI`*jU=V%BS!bD*7`iXON1z;RDC<6d#F9)vx diff --git a/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.cpp b/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.cpp index 58460d1ac7..6d29045efe 100644 --- a/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.cpp +++ b/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.cpp @@ -80,16 +80,16 @@ class AconfigStorageTest : public ::testing::Test { TEST_F(AconfigStorageTest, test_storage_version_query) { auto version = api::get_storage_file_version(package_map); ASSERT_TRUE(version.ok()); - ASSERT_EQ(*version, 2); + ASSERT_EQ(*version, 1); version = api::get_storage_file_version(flag_map); ASSERT_TRUE(version.ok()); - ASSERT_EQ(*version, 2); + ASSERT_EQ(*version, 1); version = api::get_storage_file_version(flag_val); ASSERT_TRUE(version.ok()); - ASSERT_EQ(*version, 2); + ASSERT_EQ(*version, 1); version = api::get_storage_file_version(flag_info); ASSERT_TRUE(version.ok()); - ASSERT_EQ(*version, 2); + ASSERT_EQ(*version, 1); } /// Negative test to lock down the error when mapping none exist storage files diff --git a/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.rs b/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.rs index bd1b5843f1..afc44d4d70 100644 --- a/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.rs +++ b/tools/aconfig/aconfig_storage_read_api/tests/storage_read_api_test.rs @@ -200,9 +200,9 @@ mod aconfig_storage_rust_test { #[test] fn test_storage_version_query() { - assert_eq!(get_storage_file_version("./package.map").unwrap(), 2); - assert_eq!(get_storage_file_version("./flag.map").unwrap(), 2); - assert_eq!(get_storage_file_version("./flag.val").unwrap(), 2); - assert_eq!(get_storage_file_version("./flag.info").unwrap(), 2); + assert_eq!(get_storage_file_version("./package.map").unwrap(), 1); + assert_eq!(get_storage_file_version("./flag.map").unwrap(), 1); + assert_eq!(get_storage_file_version("./flag.val").unwrap(), 1); + assert_eq!(get_storage_file_version("./flag.info").unwrap(), 1); } } From 0c6bc1ad458ce6c9e5959d9d3527c4a032a968cc Mon Sep 17 00:00:00 2001 From: Wei Li Date: Mon, 23 Sep 2024 21:24:13 +0000 Subject: [PATCH 3/4] Extract class MetadataDb to a separate python library so it can be used in notice files generation. Bug: 330949782 Test: m sbom, no diffs in generated SBOM before/after the change Test: build/soong/tests/sbom_test.sh Test: CIs Change-Id: I3cf944f744a1a5d359fd925621d1597b9681da9b --- tools/sbom/Android.bp | 8 ++ tools/sbom/compliance_metadata.py | 204 ++++++++++++++++++++++++++++++ tools/sbom/gen_sbom.py | 196 +--------------------------- 3 files changed, 217 insertions(+), 191 deletions(-) create mode 100644 tools/sbom/compliance_metadata.py diff --git a/tools/sbom/Android.bp b/tools/sbom/Android.bp index 74b3d626f6..4f6d3b7863 100644 --- a/tools/sbom/Android.bp +++ b/tools/sbom/Android.bp @@ -33,6 +33,13 @@ python_binary_host { ], } +python_library_host { + name: "compliance_metadata", + srcs: [ + "compliance_metadata.py", + ], +} + python_binary_host { name: "gen_sbom", srcs: [ @@ -44,6 +51,7 @@ python_binary_host { }, }, libs: [ + "compliance_metadata", "metadata_file_proto_py", "libprotobuf-python", "sbom_lib", diff --git a/tools/sbom/compliance_metadata.py b/tools/sbom/compliance_metadata.py new file mode 100644 index 0000000000..9910217bbe --- /dev/null +++ b/tools/sbom/compliance_metadata.py @@ -0,0 +1,204 @@ +#!/usr/bin/env python3 +# +# Copyright (C) 2024 The Android Open Source Project +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import sqlite3 + +class MetadataDb: + def __init__(self, db): + self.conn = sqlite3.connect(':memory') + self.conn.row_factory = sqlite3.Row + with sqlite3.connect(db) as c: + c.backup(self.conn) + self.reorg() + + def reorg(self): + # package_license table + self.conn.execute("create table package_license as " + "select name as package, pkg_default_applicable_licenses as license " + "from modules " + "where module_type = 'package' ") + cursor = self.conn.execute("select package,license from package_license where license like '% %'") + multi_licenses_packages = cursor.fetchall() + cursor.close() + rows = [] + for p in multi_licenses_packages: + licenses = p['license'].strip().split(' ') + for lic in licenses: + rows.append((p['package'], lic)) + self.conn.executemany('insert into package_license values (?, ?)', rows) + self.conn.commit() + + self.conn.execute("delete from package_license where license like '% %'") + self.conn.commit() + + # module_license table + self.conn.execute("create table module_license as " + "select distinct name as module, package, licenses as license " + "from modules " + "where licenses != '' ") + cursor = self.conn.execute("select module,package,license from module_license where license like '% %'") + multi_licenses_modules = cursor.fetchall() + cursor.close() + rows = [] + for m in multi_licenses_modules: + licenses = m['license'].strip().split(' ') + for lic in licenses: + rows.append((m['module'], m['package'],lic)) + self.conn.executemany('insert into module_license values (?, ?, ?)', rows) + self.conn.commit() + + self.conn.execute("delete from module_license where license like '% %'") + self.conn.commit() + + # module_installed_file table + self.conn.execute("create table module_installed_file as " + "select id as module_id, name as module_name, package, installed_files as installed_file " + "from modules " + "where installed_files != '' ") + cursor = self.conn.execute("select module_id, module_name, package, installed_file " + "from module_installed_file where installed_file like '% %'") + multi_installed_file_modules = cursor.fetchall() + cursor.close() + rows = [] + for m in multi_installed_file_modules: + installed_files = m['installed_file'].strip().split(' ') + for f in installed_files: + rows.append((m['module_id'], m['module_name'], m['package'], f)) + self.conn.executemany('insert into module_installed_file values (?, ?, ?, ?)', rows) + self.conn.commit() + + self.conn.execute("delete from module_installed_file where installed_file like '% %'") + self.conn.commit() + + # module_built_file table + self.conn.execute("create table module_built_file as " + "select id as module_id, name as module_name, package, built_files as built_file " + "from modules " + "where built_files != '' ") + cursor = self.conn.execute("select module_id, module_name, package, built_file " + "from module_built_file where built_file like '% %'") + multi_built_file_modules = cursor.fetchall() + cursor.close() + rows = [] + for m in multi_built_file_modules: + built_files = m['installed_file'].strip().split(' ') + for f in built_files: + rows.append((m['module_id'], m['module_name'], m['package'], f)) + self.conn.executemany('insert into module_built_file values (?, ?, ?, ?)', rows) + self.conn.commit() + + self.conn.execute("delete from module_built_file where built_file like '% %'") + self.conn.commit() + + + # Indexes + self.conn.execute('create index idx_modules_id on modules (id)') + self.conn.execute('create index idx_modules_name on modules (name)') + self.conn.execute('create index idx_package_licnese_package on package_license (package)') + self.conn.execute('create index idx_package_licnese_license on package_license (license)') + self.conn.execute('create index idx_module_licnese_module on module_license (module)') + self.conn.execute('create index idx_module_licnese_license on module_license (license)') + self.conn.execute('create index idx_module_installed_file_module_id on module_installed_file (module_id)') + self.conn.execute('create index idx_module_installed_file_installed_file on module_installed_file (installed_file)') + self.conn.execute('create index idx_module_built_file_module_id on module_built_file (module_id)') + self.conn.execute('create index idx_module_built_file_built_file on module_built_file (built_file)') + self.conn.commit() + + def dump_debug_db(self, debug_db): + with sqlite3.connect(debug_db) as c: + self.conn.backup(c) + + def get_installed_files(self): + # Get all records from table make_metadata, which contains all installed files and corresponding make modules' metadata + cursor = self.conn.execute('select installed_file, module_path, is_prebuilt_make_module, product_copy_files, kernel_module_copy_files, is_platform_generated, license_text from make_metadata') + rows = cursor.fetchall() + cursor.close() + installed_files_metadata = [] + for row in rows: + metadata = dict(zip(row.keys(), row)) + installed_files_metadata.append(metadata) + return installed_files_metadata + + def get_soong_modules(self): + # Get all records from table modules, which contains metadata of all soong modules + cursor = self.conn.execute('select name, package, package as module_path, module_type as soong_module_type, built_files, installed_files, static_dep_files, whole_static_dep_files from modules') + rows = cursor.fetchall() + cursor.close() + soong_modules = [] + for row in rows: + soong_module = dict(zip(row.keys(), row)) + soong_modules.append(soong_module) + return soong_modules + + def get_package_licenses(self, package): + cursor = self.conn.execute('select m.name, m.package, m.lic_license_text as license_text ' + 'from package_license pl join modules m on pl.license = m.name ' + 'where pl.package = ?', + ('//' + package,)) + rows = cursor.fetchall() + licenses = {} + for r in rows: + licenses[r['name']] = r['license_text'] + return licenses + + def get_module_licenses(self, module_name, package): + licenses = {} + # If property "licenses" is defined on module + cursor = self.conn.execute('select m.name, m.package, m.lic_license_text as license_text ' + 'from module_license ml join modules m on ml.license = m.name ' + 'where ml.module = ? and ml.package = ?', + (module_name, package)) + rows = cursor.fetchall() + for r in rows: + licenses[r['name']] = r['license_text'] + if len(licenses) > 0: + return licenses + + # Use default package license + cursor = self.conn.execute('select m.name, m.package, m.lic_license_text as license_text ' + 'from package_license pl join modules m on pl.license = m.name ' + 'where pl.package = ?', + ('//' + package,)) + rows = cursor.fetchall() + for r in rows: + licenses[r['name']] = r['license_text'] + return licenses + + def get_soong_module_of_installed_file(self, installed_file): + cursor = self.conn.execute('select name, m.package, m.package as module_path, module_type as soong_module_type, built_files, installed_files, static_dep_files, whole_static_dep_files ' + 'from modules m join module_installed_file mif on m.id = mif.module_id ' + 'where mif.installed_file = ?', + (installed_file,)) + rows = cursor.fetchall() + cursor.close() + if rows: + soong_module = dict(zip(rows[0].keys(), rows[0])) + return soong_module + + return None + + def get_soong_module_of_built_file(self, built_file): + cursor = self.conn.execute('select name, m.package, m.package as module_path, module_type as soong_module_type, built_files, installed_files, static_dep_files, whole_static_dep_files ' + 'from modules m join module_built_file mbf on m.id = mbf.module_id ' + 'where mbf.built_file = ?', + (built_file,)) + rows = cursor.fetchall() + cursor.close() + if rows: + soong_module = dict(zip(rows[0].keys(), rows[0])) + return soong_module + + return None \ No newline at end of file diff --git a/tools/sbom/gen_sbom.py b/tools/sbom/gen_sbom.py index a203258b96..9c3a8be9ef 100644 --- a/tools/sbom/gen_sbom.py +++ b/tools/sbom/gen_sbom.py @@ -26,6 +26,7 @@ Usage example: """ import argparse +import compliance_metadata import datetime import google.protobuf.text_format as text_format import hashlib @@ -35,7 +36,6 @@ import queue import metadata_file_pb2 import sbom_data import sbom_writers -import sqlite3 # Package type PKG_SOURCE = 'SOURCE' @@ -568,202 +568,16 @@ def get_all_transitive_static_dep_files_of_installed_files(installed_files_metad return sorted(all_static_dep_files.keys()) -class MetadataDb: - def __init__(self, db): - self.conn = sqlite3.connect(':memory') - self.conn.row_factory = sqlite3.Row - with sqlite3.connect(db) as c: - c.backup(self.conn) - self.reorg() - - def reorg(self): - # package_license table - self.conn.execute("create table package_license as " - "select name as package, pkg_default_applicable_licenses as license " - "from modules " - "where module_type = 'package' ") - cursor = self.conn.execute("select package,license from package_license where license like '% %'") - multi_licenses_packages = cursor.fetchall() - cursor.close() - rows = [] - for p in multi_licenses_packages: - licenses = p['license'].strip().split(' ') - for lic in licenses: - rows.append((p['package'], lic)) - self.conn.executemany('insert into package_license values (?, ?)', rows) - self.conn.commit() - - self.conn.execute("delete from package_license where license like '% %'") - self.conn.commit() - - # module_license table - self.conn.execute("create table module_license as " - "select distinct name as module, package, licenses as license " - "from modules " - "where licenses != '' ") - cursor = self.conn.execute("select module,package,license from module_license where license like '% %'") - multi_licenses_modules = cursor.fetchall() - cursor.close() - rows = [] - for m in multi_licenses_modules: - licenses = m['license'].strip().split(' ') - for lic in licenses: - rows.append((m['module'], m['package'],lic)) - self.conn.executemany('insert into module_license values (?, ?, ?)', rows) - self.conn.commit() - - self.conn.execute("delete from module_license where license like '% %'") - self.conn.commit() - - # module_installed_file table - self.conn.execute("create table module_installed_file as " - "select id as module_id, name as module_name, package, installed_files as installed_file " - "from modules " - "where installed_files != '' ") - cursor = self.conn.execute("select module_id, module_name, package, installed_file " - "from module_installed_file where installed_file like '% %'") - multi_installed_file_modules = cursor.fetchall() - cursor.close() - rows = [] - for m in multi_installed_file_modules: - installed_files = m['installed_file'].strip().split(' ') - for f in installed_files: - rows.append((m['module_id'], m['module_name'], m['package'], f)) - self.conn.executemany('insert into module_installed_file values (?, ?, ?, ?)', rows) - self.conn.commit() - - self.conn.execute("delete from module_installed_file where installed_file like '% %'") - self.conn.commit() - - # module_built_file table - self.conn.execute("create table module_built_file as " - "select id as module_id, name as module_name, package, built_files as built_file " - "from modules " - "where built_files != '' ") - cursor = self.conn.execute("select module_id, module_name, package, built_file " - "from module_built_file where built_file like '% %'") - multi_built_file_modules = cursor.fetchall() - cursor.close() - rows = [] - for m in multi_built_file_modules: - built_files = m['installed_file'].strip().split(' ') - for f in built_files: - rows.append((m['module_id'], m['module_name'], m['package'], f)) - self.conn.executemany('insert into module_built_file values (?, ?, ?, ?)', rows) - self.conn.commit() - - self.conn.execute("delete from module_built_file where built_file like '% %'") - self.conn.commit() - - - # Indexes - self.conn.execute('create index idx_modules_id on modules (id)') - self.conn.execute('create index idx_modules_name on modules (name)') - self.conn.execute('create index idx_package_licnese_package on package_license (package)') - self.conn.execute('create index idx_package_licnese_license on package_license (license)') - self.conn.execute('create index idx_module_licnese_module on module_license (module)') - self.conn.execute('create index idx_module_licnese_license on module_license (license)') - self.conn.execute('create index idx_module_installed_file_module_id on module_installed_file (module_id)') - self.conn.execute('create index idx_module_installed_file_installed_file on module_installed_file (installed_file)') - self.conn.execute('create index idx_module_built_file_module_id on module_built_file (module_id)') - self.conn.execute('create index idx_module_built_file_built_file on module_built_file (built_file)') - self.conn.commit() - - if args.debug: - with sqlite3.connect(os.path.dirname(args.metadata) + '/compliance-metadata-debug.db') as c: - self.conn.backup(c) - - - def get_installed_files(self): - # Get all records from table make_metadata, which contains all installed files and corresponding make modules' metadata - cursor = self.conn.execute('select installed_file, module_path, is_prebuilt_make_module, product_copy_files, kernel_module_copy_files, is_platform_generated, license_text from make_metadata') - rows = cursor.fetchall() - cursor.close() - installed_files_metadata = [] - for row in rows: - metadata = dict(zip(row.keys(), row)) - installed_files_metadata.append(metadata) - return installed_files_metadata - - def get_soong_modules(self): - # Get all records from table modules, which contains metadata of all soong modules - cursor = self.conn.execute('select name, package, package as module_path, module_type as soong_module_type, built_files, installed_files, static_dep_files, whole_static_dep_files from modules') - rows = cursor.fetchall() - cursor.close() - soong_modules = [] - for row in rows: - soong_module = dict(zip(row.keys(), row)) - soong_modules.append(soong_module) - return soong_modules - - def get_package_licenses(self, package): - cursor = self.conn.execute('select m.name, m.package, m.lic_license_text as license_text ' - 'from package_license pl join modules m on pl.license = m.name ' - 'where pl.package = ?', - ('//' + package,)) - rows = cursor.fetchall() - licenses = {} - for r in rows: - licenses[r['name']] = r['license_text'] - return licenses - - def get_module_licenses(self, module_name, package): - licenses = {} - # If property "licenses" is defined on module - cursor = self.conn.execute('select m.name, m.package, m.lic_license_text as license_text ' - 'from module_license ml join modules m on ml.license = m.name ' - 'where ml.module = ? and ml.package = ?', - (module_name, package)) - rows = cursor.fetchall() - for r in rows: - licenses[r['name']] = r['license_text'] - if len(licenses) > 0: - return licenses - - # Use default package license - cursor = self.conn.execute('select m.name, m.package, m.lic_license_text as license_text ' - 'from package_license pl join modules m on pl.license = m.name ' - 'where pl.package = ?', - ('//' + package,)) - rows = cursor.fetchall() - for r in rows: - licenses[r['name']] = r['license_text'] - return licenses - - def get_soong_module_of_installed_file(self, installed_file): - cursor = self.conn.execute('select name, m.package, m.package as module_path, module_type as soong_module_type, built_files, installed_files, static_dep_files, whole_static_dep_files ' - 'from modules m join module_installed_file mif on m.id = mif.module_id ' - 'where mif.installed_file = ?', - (installed_file,)) - rows = cursor.fetchall() - cursor.close() - if rows: - soong_module = dict(zip(rows[0].keys(), rows[0])) - return soong_module - - return None - - def get_soong_module_of_built_file(self, built_file): - cursor = self.conn.execute('select name, m.package, m.package as module_path, module_type as soong_module_type, built_files, installed_files, static_dep_files, whole_static_dep_files ' - 'from modules m join module_built_file mbf on m.id = mbf.module_id ' - 'where mbf.built_file = ?', - (built_file,)) - rows = cursor.fetchall() - cursor.close() - if rows: - soong_module = dict(zip(rows[0].keys(), rows[0])) - return soong_module - - return None - - def main(): global args args = get_args() log('Args:', vars(args)) global db - db = MetadataDb(args.metadata) + db = compliance_metadata.MetadataDb(args.metadata) + if args.debug: + db.dump_debug_db(os.path.dirname(args.output_file) + '/compliance-metadata-debug.db') + global metadata_file_protos metadata_file_protos = {} global licenses_text From 4d48559e2d743ff7b1c29d4f4e16b3a6ceee3ef7 Mon Sep 17 00:00:00 2001 From: Zhuoyao Zhang Date: Tue, 17 Sep 2024 21:14:38 +0000 Subject: [PATCH 4/4] Ensure a single running instance of edit monitor This cl ensures at most 1 instance of edit monitor running from the same binary by killing any existing instance before starting. Specifically, When an edit monitor process starts, it will write a pidfile containing its pid and if such pidfile already exists (which means there's another instance there), it will read the pid contained in that pidfile and kill the corresponding process first. Test: atst daemon_manager_test bug: 365617369 Change-Id: I76954344df649aa79a6ef07ce55a62985decdb53 --- tools/edit_monitor/daemon_manager.py | 17 ++++ tools/edit_monitor/daemon_manager_test.py | 96 ++++++++++++++++++----- 2 files changed, 93 insertions(+), 20 deletions(-) diff --git a/tools/edit_monitor/daemon_manager.py b/tools/edit_monitor/daemon_manager.py index c09c3213ae..8ec25886dc 100644 --- a/tools/edit_monitor/daemon_manager.py +++ b/tools/edit_monitor/daemon_manager.py @@ -55,6 +55,7 @@ class DaemonManager: def start(self): """Writes the pidfile and starts the daemon proces.""" try: + self._stop_any_existing_instance() self._write_pid_to_pidfile() self._start_daemon_process() except Exception as e: @@ -71,6 +72,22 @@ class DaemonManager: except Exception as e: logging.exception("Failed to stop daemon manager with error %s", e) + def _stop_any_existing_instance(self): + if not self.pid_file_path.exists(): + logging.debug("No existing instances.") + return + + ex_pid = self._read_pid_from_pidfile() + + if ex_pid: + logging.info("Found another instance with pid %d.", ex_pid) + self._terminate_process(ex_pid) + self._remove_pidfile() + + def _read_pid_from_pidfile(self): + with open(self.pid_file_path, "r") as f: + return int(f.read().strip()) + def _write_pid_to_pidfile(self): """Creates a pidfile and writes the current pid to the file. diff --git a/tools/edit_monitor/daemon_manager_test.py b/tools/edit_monitor/daemon_manager_test.py index 5be4beee51..214b0388dc 100644 --- a/tools/edit_monitor/daemon_manager_test.py +++ b/tools/edit_monitor/daemon_manager_test.py @@ -33,7 +33,7 @@ TEST_PID_FILE_PATH = ( ) -def example_daemon(output_file): +def simple_daemon(output_file): with open(output_file, 'w') as f: f.write('running daemon target') @@ -69,29 +69,62 @@ class DaemonManagerTest(unittest.TestCase): tempfile.tempdir = self.original_tempdir super().tearDown() - def test_start_success(self): - damone_output_file = tempfile.NamedTemporaryFile( - dir=self.working_dir.name, delete=False + def test_start_success_with_no_existing_instance(self): + self.assert_run_simple_daemon_success() + + def test_start_success_with_existing_instance_running(self): + # Create a long running subprocess + p = multiprocessing.Process(target=long_running_daemon) + p.start() + + # Create a pidfile with the subprocess pid + pid_file_path_dir = pathlib.Path(self.working_dir.name).joinpath( + 'edit_monitor' ) - dm = daemon_manager.DaemonManager( - TEST_BINARY_FILE, - daemon_target=example_daemon, - daemon_args=(damone_output_file.name,), + pid_file_path_dir.mkdir(parents=True, exist_ok=True) + with open(pid_file_path_dir.joinpath(TEST_PID_FILE_PATH), 'w') as f: + f.write(str(p.pid)) + + self.assert_run_simple_daemon_success() + p.terminate() + + def test_start_success_with_existing_instance_already_dead(self): + # Create a pidfile with pid that does not exist. + pid_file_path_dir = pathlib.Path(self.working_dir.name).joinpath( + 'edit_monitor' ) + pid_file_path_dir.mkdir(parents=True, exist_ok=True) + with open(pid_file_path_dir.joinpath(TEST_PID_FILE_PATH), 'w') as f: + f.write('123456') + + self.assert_run_simple_daemon_success() + + def test_start_success_with_existing_instance_from_different_binary(self): + # First start an instance based on "some_binary_path" + existing_dm = daemon_manager.DaemonManager( + "some_binary_path", + daemon_target=long_running_daemon, + ) + existing_dm.start() + + self.assert_run_simple_daemon_success() + existing_dm.stop() + + @mock.patch('os.kill') + def test_start_failed_to_kill_existing_instance(self, mock_kill): + mock_kill.side_effect = OSError('Unknown OSError') + pid_file_path_dir = pathlib.Path(self.working_dir.name).joinpath( + 'edit_monitor' + ) + pid_file_path_dir.mkdir(parents=True, exist_ok=True) + with open(pid_file_path_dir.joinpath(TEST_PID_FILE_PATH), 'w') as f: + f.write('123456') + + dm = daemon_manager.DaemonManager(TEST_BINARY_FILE) dm.start() - dm.daemon_process.join() - # Verifies the expected pid file is created. - expected_pid_file_path = pathlib.Path(self.working_dir.name).joinpath( - 'edit_monitor', TEST_PID_FILE_PATH - ) - self.assertEqual(dm.pid_file_path, expected_pid_file_path) - self.assertTrue(expected_pid_file_path.exists()) - - # Verifies the daemon process is executed successfully. - with open(damone_output_file.name, 'r') as f: - contents = f.read() - self.assertEqual(contents, 'running daemon target') + # Verify no daemon process is started. + self.assertIsNone(dm.daemon_process) def test_start_failed_to_write_pidfile(self): pid_file_path_dir = pathlib.Path(self.working_dir.name).joinpath( @@ -151,6 +184,29 @@ class DaemonManagerTest(unittest.TestCase): self.assert_no_subprocess_running() self.assertTrue(dm.pid_file_path.exists()) + def assert_run_simple_daemon_success(self): + damone_output_file = tempfile.NamedTemporaryFile( + dir=self.working_dir.name, delete=False + ) + dm = daemon_manager.DaemonManager( + TEST_BINARY_FILE, + daemon_target=simple_daemon, + daemon_args=(damone_output_file.name,), + ) + dm.start() + dm.daemon_process.join() + + # Verifies the expected pid file is created. + expected_pid_file_path = pathlib.Path(self.working_dir.name).joinpath( + 'edit_monitor', TEST_PID_FILE_PATH + ) + self.assertTrue(expected_pid_file_path.exists()) + + # Verify the daemon process is executed successfully. + with open(damone_output_file.name, 'r') as f: + contents = f.read() + self.assertEqual(contents, 'running daemon target') + def assert_no_subprocess_running(self): child_pids = self._get_child_processes(os.getpid()) for child_pid in child_pids: