Enable clippy for android code

This change enables clippy builds of android specific code.

Doing so without adding the full Android SDK into our container
is a bit hacky. The CL https://crrev.com/c/5671653 adds env variables
to the minijail build.rs to allow us to skip building the library, and
generate bindings for linux instead of android.

This allow us to build all non-gpu related features of the
android build. It will not link, but it will run clippy.

This CL fixes various clippy issues that come up in this new
configuration

BUG=b:349907813
TEST=tools/clippy -p android
TEST=tools/presubmit clippy_android

Change-Id: I1f51d383051bbeeeff55d716feb7b56c3e24588b
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/5672567
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Dennis Kempin <denniskempin@google.com>

.cargo/config.toml

@@ -16,6 +16,7 @@ rustflags = [
     "-Aclippy::needless_bool",
     "-Aclippy::new-ret-no-self",
     "-Aclippy::or_fun_call",
+    "-Aclippy::result_large_err",
     "-Aclippy::result-unit-err",
     "-Aclippy::should_implement_trait",
     "-Aclippy::single_char_pattern",

Cargo.lock

@@ -70,6 +70,12 @@ dependencies = [
  "thiserror",
 ]
 
+[[package]]
+name = "android_log-sys"
+version = "0.3.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "5ecc8056bf6ab9892dcd53216c83d1597487d7dacac16c8df6b877d127df9937"
+
 [[package]]
 name = "anti_tamper"
 version = "0.1.0"
@@ -266,6 +272,7 @@ dependencies = [
 name = "base"
 version = "0.1.0"
 dependencies = [
+ "android_log-sys",
  "audio_streams",
  "base_event_token_derive",
  "cfg-if",

Cargo.toml

@@ -435,6 +435,25 @@ all-mingw64 = [
 ## All features that are compiled and tested for msvc64
 all-msvc64 = [ "all-mingw64" ]
 
+## All features that are compiled and tested for android builds
+all-android = [
+        "android-sparse",
+        "audio",
+        "audio_aaudio",
+        "balloon",
+        "config-file",
+        "gdb",
+        "gdbstub",
+        "gdbstub_arch",
+        "geniezone",
+        "gunyah",
+        "libaaudio_stub",
+        "net",
+        "qcow",
+        "usb",
+        "composite-disk",
+]
+
 [dependencies]
 anyhow = "1.0.32"
 arch = { path = "arch" }
@@ -464,7 +483,7 @@ kernel_cmdline = { path = "kernel_cmdline" }
 kernel_loader = { path = "kernel_loader" }
 kvm = { path = "kvm", optional = true }
 kvm_sys = { path = "kvm_sys", optional = true }
-libc = "0.2.93"
+libc = "0.2.153"
 libcras = "*"
 # Compile out trace statements in release builds
 log = { version = "0", features = ["release_max_level_debug"]}

base/Cargo.toml

@@ -38,3 +38,6 @@ protobuf = "3.2"
 rand = "0.8"
 winapi = "0.3"
 win_util = { path = "../win_util"}
+
+[target.'cfg(target_os = "android")'.dependencies]
+android_log-sys = "0.3.1"

base/src/mmap.rs

@@ -30,6 +30,12 @@ use crate::VolatileMemoryError;
 use crate::VolatileMemoryResult;
 use crate::VolatileSlice;
 
+// TODO: Remove once available in libc bindings
+#[cfg(target_os = "android")]
+const _SC_LEVEL1_DCACHE_LINESIZE: i32 = 0x0094;
+#[cfg(target_os = "linux")]
+use libc::_SC_LEVEL1_DCACHE_LINESIZE;
+
 static CACHELINE_SIZE: OnceLock<usize> = OnceLock::new();
 
 #[allow(unused_assignments)]
@@ -39,7 +45,7 @@ fn get_cacheline_size_once() -> usize {
         if #[cfg(all(any(target_os = "android", target_os = "linux"), not(target_env = "musl")))] {
             // SAFETY:
             // Safe because we check the return value for errors or unsupported requests
-            let linesize = unsafe { libc::sysconf(libc::_SC_LEVEL1_DCACHE_LINESIZE) };
+            let linesize = unsafe { libc::sysconf(_SC_LEVEL1_DCACHE_LINESIZE) };
             if linesize > 0 {
                 return linesize as usize;
             } else {

base/src/sys/linux/android/syslog.rs

@@ -4,8 +4,6 @@
 
 //! Implementation of the Syslog trait as a wrapper around Android's logging library, liblog.
 
-extern crate android_log_sys;
-
 use std::ffi::CString;
 use std::ffi::NulError;
 use std::mem::size_of;
@@ -30,7 +28,7 @@ pub struct PlatformSyslog {
 impl Syslog for PlatformSyslog {
     fn new(
         proc_name: String,
-        facility: Facility,
+        _facility: Facility,
     ) -> Result<(Option<Box<dyn Log + Send>>, Option<RawDescriptor>), Error> {
         Ok((Some(Box::new(Self { proc_name })), None))
     }
@@ -56,7 +54,7 @@ impl Log for PlatformSyslog {
         );
     }
 
-    fn enabled(&self, metadata: &log::Metadata) -> bool {
+    fn enabled(&self, _metadata: &log::Metadata) -> bool {
         true
     }
 
@@ -82,6 +80,7 @@ fn android_log(
 ) -> Result<(), NulError> {
     let tag = CString::new(tag)?;
     let default_pri = LogPriority::VERBOSE;
+    // SAFETY: `tag` is guaranteed to be valid for duration of the call
     if unsafe { __android_log_is_loggable(priority as i32, tag.as_ptr(), default_pri as i32) } != 0
     {
         let c_file_name = match file {
@@ -99,6 +98,7 @@ fn android_log(
             line,
             message: message.as_ptr(),
         };
+        // SAFETY: `log_message` is guaranteed to be valid for duration of the call
         unsafe { __android_log_write_log_message(&mut log_message) };
     }
     Ok(())

base/src/sys/linux/sched.rs

@@ -35,6 +35,7 @@ impl CpuSet {
         CpuSet(cpuset)
     }
 
+    #[allow(clippy::unnecessary_cast)]
     pub fn to_cpus(&self) -> Vec<usize> {
         let mut cpus = Vec::new();
         for i in 0..(CPU_SETSIZE as usize) {
@@ -70,6 +71,7 @@ impl FromIterator<usize> for CpuSet {
 /// # use base::linux::set_cpu_affinity;
 ///   set_cpu_affinity(vec![0, 1, 5, 6]).unwrap();
 /// ```
+#[allow(clippy::unnecessary_cast)]
 pub fn set_cpu_affinity<I: IntoIterator<Item = usize>>(cpus: I) -> Result<()> {
     let CpuSet(cpuset) = cpus
         .into_iter()

devices/src/vfio.rs

@@ -225,7 +225,7 @@ impl KvmVfioPviommu {
         if ret < 0 {
             Err(VfioError::KvmSetDeviceAttr(get_error()))
         } else {
-            // Safe as we verify the return value.
+            // SAFETY: Safe as we verify the return value.
             Ok(unsafe { File::from_raw_descriptor(ret) })
         }
     }

devices/src/virtio/vhost/user/device/fs/sys/linux.rs

@@ -2,7 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-use std::io;
 use std::path::Path;
 use std::path::PathBuf;
 
@@ -30,6 +29,7 @@ fn default_gidmap() -> String {
     format!("{} {} 1", egid, egid)
 }
 
+#[allow(clippy::unnecessary_cast)]
 fn jail_and_fork(
     mut keep_rds: Vec<RawDescriptor>,
     dir_path: PathBuf,
@@ -122,26 +122,26 @@ pub fn start_device(opts: Options) -> anyhow::Result<()> {
         return Ok(());
     }
 
-    // We need to set the no setuid fixup secure bit so that we don't drop capabilities when
-    // changing the thread uid/gid. Without this, creating new entries can fail in some corner
-    // cases.
-    const SECBIT_NO_SETUID_FIXUP: i32 = 1 << 2;
-
     // TODO(crbug.com/1199487): Remove this once libc provides the wrapper for all targets.
     #[cfg(target_os = "linux")]
     {
+        // We need to set the no setuid fixup secure bit so that we don't drop capabilities when
+        // changing the thread uid/gid. Without this, creating new entries can fail in some corner
+        // cases.
+        const SECBIT_NO_SETUID_FIXUP: i32 = 1 << 2;
+
         // SAFETY:
         // Safe because this doesn't modify any memory and we check the return value.
         let mut securebits = unsafe { libc::prctl(libc::PR_GET_SECUREBITS) };
         if securebits < 0 {
-            bail!(io::Error::last_os_error());
+            bail!(std::io::Error::last_os_error());
         }
         securebits |= SECBIT_NO_SETUID_FIXUP;
         // SAFETY:
         // Safe because this doesn't modify any memory and we check the return value.
         let ret = unsafe { libc::prctl(libc::PR_SET_SECUREBITS, securebits) };
         if ret < 0 {
-            bail!(io::Error::last_os_error());
+            bail!(std::io::Error::last_os_error());
         }
     }
 

devices/src/virtio/wl.rs

@@ -72,7 +72,6 @@ use base::Error;
 use base::Event;
 use base::EventToken;
 use base::EventType;
-use base::FromRawDescriptor;
 #[cfg(feature = "gpu")]
 use base::IntoRawDescriptor;
 #[cfg(feature = "minigbm")]
@@ -464,7 +463,7 @@ struct VmRequester {
 fn to_safe_descriptor(r: RutabagaDescriptor) -> SafeDescriptor {
     // SAFETY:
     // Safe because we own the SafeDescriptor at this point.
-    unsafe { SafeDescriptor::from_raw_descriptor(r.into_raw_descriptor()) }
+    unsafe { base::FromRawDescriptor::from_raw_descriptor(r.into_raw_descriptor()) }
 }
 
 impl VmRequester {
@@ -1494,7 +1493,9 @@ impl WlState {
                             *descriptor = dup.into_raw_descriptor();
                             // SAFETY:
                             // Safe because the fd comes from a valid SafeDescriptor.
-                            let file = unsafe { File::from_raw_descriptor(*descriptor) };
+                            let file: File = unsafe {
+                                base::FromRawDescriptor::from_raw_descriptor(*descriptor)
+                            };
                             bridged_files.push(file);
                         }
                         Err(_) => return Ok(WlResp::InvalidId),

jail/src/helpers.rs

@@ -110,6 +110,7 @@ impl Drop for ScopedMinijail {
 ///
 /// * `root` - The root path to be changed to by minijail.
 /// * `max_open_files` - The maximum number of file descriptors to allow a jailed process to open.
+#[allow(clippy::unnecessary_cast)]
 pub fn create_base_minijail(root: &Path, max_open_files: u64) -> Result<Minijail> {
     // Validate new root directory. Path::is_dir() also checks the existence.
     if !root.is_dir() {

src/crosvm/sys/linux/android.rs

@@ -4,7 +4,6 @@
 
 #![cfg(target_os = "android")]
 
-use std::ffi::CStr;
 use std::ffi::CString;
 
 use anyhow::anyhow;
@@ -22,8 +21,8 @@ extern "C" {
 }
 
 // Apply the listed task profiles to all tasks (current and future) in this process.
-pub fn set_process_profiles(profiles: &Vec<String>) -> Result<()> {
+pub fn set_process_profiles(profiles: &[String]) -> Result<()> {
-    if (profiles.is_empty()) {
+    if profiles.is_empty() {
         return Ok(());
     }
     let owned: Vec<CString> = profiles
@@ -34,8 +33,7 @@ pub fn set_process_profiles(profiles: &Vec<String>) -> Result<()> {
     // SAFETY: the ownership of the array of string is not passed. The function copies it
     // internally.
     unsafe {
-        if (android_set_process_profiles(libc::getuid(), libc::getpid(), ptrs.len(), ptrs.as_ptr()))
+        if android_set_process_profiles(libc::getuid(), libc::getpid(), ptrs.len(), ptrs.as_ptr()) {
-        {
             Ok(())
         } else {
             Err(anyhow!("failed to set task profiles"))

src/crosvm/sys/linux/device_helpers.rs

@@ -409,7 +409,7 @@ pub fn create_virtio_snd_device(
         Backend::Sys(virtio::snd::sys::StreamSourceBackend::AAUDIO) => "snd_aaudio_device",
         #[cfg(feature = "audio_cras")]
         Backend::Sys(virtio::snd::sys::StreamSourceBackend::CRAS) => "snd_cras_device",
-        #[cfg(not(feature = "audio_cras"))]
+        #[cfg(not(any(feature = "audio_cras", feature = "audio_aaudio")))]
         _ => unreachable!(),
     };
 

src/crosvm/sys/linux/vcpu.rs

@@ -37,8 +37,6 @@ use hypervisor::IoParams;
 use hypervisor::VcpuExit;
 use hypervisor::VcpuSignalHandle;
 use libc::c_int;
-use libc::SCHED_FLAG_KEEP_ALL;
-use libc::SCHED_FLAG_RESET_ON_FORK;
 use metrics_events::MetricEventType;
 #[cfg(target_arch = "riscv64")]
 use riscv64::Riscv64 as Arch;
@@ -55,8 +53,12 @@ use super::ExitState;
 use crate::crosvm::ratelimit::Ratelimit;
 
 // TODO(davidai): Import libc constant when updated
+const SCHED_FLAG_RESET_ON_FORK: u64 = 0x1;
+const SCHED_FLAG_KEEP_POLICY: u64 = 0x08;
+const SCHED_FLAG_KEEP_PARAMS: u64 = 0x10;
 const SCHED_FLAG_UTIL_CLAMP_MIN: u64 = 0x20;
 const SCHED_SCALE_CAPACITY: u32 = 1024;
+const SCHED_FLAG_KEEP_ALL: u64 = SCHED_FLAG_KEEP_POLICY | SCHED_FLAG_KEEP_PARAMS;
 
 fn bus_io_handler(bus: &Bus) -> impl FnMut(IoParams) -> Option<[u8; 8]> + '_ {
     |IoParams {
@@ -89,6 +91,7 @@ fn bus_io_handler(bus: &Bus) -> impl FnMut(IoParams) -> Option<[u8; 8]> + '_ {
 
 /// Set the VCPU thread affinity and other per-thread scheduler properties.
 /// This function will be called from each VCPU thread at startup.
+#[allow(clippy::unnecessary_cast)]
 pub fn set_vcpu_thread_scheduling(
     vcpu_affinity: CpuSet,
     core_scheduling: bool,

third_party/minijail

@@ -1 +1 @@
-Subproject commit 13be56d79718425a60173f61f8174669d9cc8930
+Subproject commit 76b25b89fc9035f39df32b59c870cd8111ad5348

tools/clippy

@@ -30,7 +30,7 @@ def main(
     chdir(CROSVM_ROOT)
 
     # Note: Clippy checks are configured in .cargo/config.toml
-    common_args = [
+    args = [
         "--message-format=json" if json else None,
         "--locked" if locked else None,
         "--all-targets",
@@ -38,22 +38,23 @@ def main(
         "-Dwarnings",
     ]
     if fix:
-        common_args = [
+        args = [
             "--fix",
             "--allow-no-vcs",
             "--allow-dirty",
             "--allow-staged",
-            *common_args,
+            *args,
         ]
+    # For experimental builds, don't clippy the whole workspace, just what's enabled by features.
+    if triple in (Triple.from_shorthand("riscv64"), Triple.from_shorthand("android")):
+        args = ["--no-default-features", *args]
+    else:
+        args = ["--workspace", *args]
+
     print("Clippy crosvm workspace")
-    exclude_args = []
-    if triple == Triple.from_shorthand("riscv64"):
-        exclude_args = ["--exclude=" + s for s in DO_NOT_BUILD_RISCV64]
     clippy(
-        "--workspace",
         f"--features={triple.feature_flag}",
-        *exclude_args,
+        *args,
-        *common_args,
     ).with_envs(triple.get_cargo_env()).fg()
 
 

tools/custom_checks

@@ -128,6 +128,7 @@ def check_rust_features(*files: str):
             *collect_features("all-mingw64"),
             *collect_features("all-msvc64"),
             *collect_features("all-riscv64"),
+            *collect_features("all-android"),
         )
     )
     disabled_features = [

tools/impl/util.py

@@ -210,6 +210,7 @@ SHORTHANDS = {
     "aarch64": "aarch64-unknown-linux-gnu",
     "riscv64": "riscv64gc-unknown-linux-gnu",
     "x86_64": "x86_64-unknown-linux-gnu",
+    "android": "aarch64-linux-android",
 }
 
 
@@ -285,10 +286,22 @@ class Triple(NamedTuple):
         env["CARGO_BUILD_TARGET"] = cargo_target
         env["CARGO_TARGET_DIR"] = str(self.target_dir)
         env["CROSVM_TARGET_DIR"] = str(crosvm_target_dir())
+        # Android builds are not fully supported and can only be used to run clippy.
+        # Underlying libraries (e.g. minijail) will be built for linux instead
+        # TODO(denniskempin): This could be better done with [env] in Cargo.toml if it supported
+        # per-target configuration. See https://github.com/rust-lang/cargo/issues/10273
+        if str(self).endswith("-linux-android"):
+            env["MINIJAIL_DO_NOT_BUILD"] = "true"
+            env["MINIJAIL_BINDGEN_TARGET"] = f"{self.arch}-unknown-linux-gnu"
         return env
 
     def __str__(self):
-        return f"{self.arch}-{self.vendor}-{self.sys}-{self.abi}"
+        parts = [self.arch, self.vendor]
+        if self.sys:
+            parts = [*parts, self.sys]
+        if self.abi:
+            parts = [*parts, self.abi]
+        return "-".join(parts)
 
 
 def download_file(url: str, filename: Path, attempts: int = 3):

tools/presubmit

@@ -5,7 +5,7 @@
 
 import os
 import typing
-from typing import Generator, List, Literal, Optional, Tuple
+from typing import Generator, List, Literal, Optional, Tuple, Union
 
 from impl.common import (
     CROSVM_ROOT,
@@ -28,8 +28,11 @@ lucicfg = cmd("third_party/depot_tools/lucicfg")
 Platform = Literal["x86_64", "aarch64", "mingw64", "armhf"]
 PLATFORMS: Tuple[Platform, ...] = typing.get_args(Platform)
 
+ClippyOnlyPlatform = Literal["android"]
+CLIPPY_ONLY_PLATFORMS: Tuple[ClippyOnlyPlatform, ...] = typing.get_args(ClippyOnlyPlatform)
 
-def platform_is_supported(platform: Platform):
+
+def platform_is_supported(platform: Union[Platform, ClippyOnlyPlatform]):
     "Returns true if the platform is available as a target in rustup."
     triple = Triple.from_shorthand(platform)
     installed_toolchains = cmd("rustup target list --installed").lines()
@@ -158,7 +161,7 @@ def check_crosvm_build(
     return check
 
 
-def check_clippy(platform: Platform):
+def check_clippy(platform: Union[Platform, ClippyOnlyPlatform]):
     def check(context: CheckContext):
         if not platform_is_supported(platform):
             return None
@@ -294,7 +297,7 @@ CHECKS: List[Check] = [
             can_fix=True,
             priority=True,
         )
-        for platform in PLATFORMS
+        for platform in (*PLATFORMS, *CLIPPY_ONLY_PLATFORMS)
     ),
     Check(
         custom_check("check-copyright-header"),
@@ -373,13 +376,14 @@ GROUPS: List[Group] = [
         checks=[
             "health_checks",
             *(f"linux_{platform}" for platform in PLATFORMS),
+            *(f"clippy_{platform}" for platform in CLIPPY_ONLY_PLATFORMS),
         ],
     ),
     # Convenience groups for local usage:
     Group(
         name="clippy",
         doc="Runs clippy for all platforms",
-        checks=[f"clippy_{platform}" for platform in PLATFORMS],
+        checks=[f"clippy_{platform}" for platform in PLATFORMS + CLIPPY_ONLY_PLATFORMS],
     ),
     Group(
         name="unit_tests",

vm_control/src/lib.rs

@@ -1628,6 +1628,7 @@ impl VmRequest {
     /// This does not return a result, instead encapsulating the success or failure in a
     /// `VmResponse` with the intended purpose of sending the response back over the  socket that
     /// received this `VmRequest`.
+    #[allow(unused_variables)]
     pub fn execute(
         &self,
         vm: &impl Vm,