From 210f39a46f275b120c8cdee6bb4c5f483ffcb9a6 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Thu, 22 Apr 2021 11:27:12 +1000 Subject: [PATCH 01/12] agent/rustjail: Simplify renaming imports Functions in rustjail deal with both the local oci module's data structure and the protocol::oci module's data structure. Since these both cover the OCI container config they are quite similar and have many identically named types. To avoid conflicts, we import many things from those modules with altered names. However the names we use oci* and grpc* don't fit the normal Rust capitalization convention for types. However by renaming the import of the 'protocols::oci' module itself to 'grpc', we can actually get rid of the many renames by just qualifying at each use site with only a very small increase in verbosity. As a bonus this gets rid of multiple 'use' items scattered through the file. Signed-off-by: David Gibson --- src/agent/rustjail/src/lib.rs | 137 +++++++++++++--------------------- 1 file changed, 53 insertions(+), 84 deletions(-) diff --git a/src/agent/rustjail/src/lib.rs b/src/agent/rustjail/src/lib.rs index 4b66b8a05c..a694f2390c 100644 --- a/src/agent/rustjail/src/lib.rs +++ b/src/agent/rustjail/src/lib.rs @@ -58,24 +58,17 @@ pub mod validator; // pub mod user; //pub mod intelrdt; -// construtc ociSpec from grpcSpec, which is needed for hook +use protocols::oci as grpc; + +// construtc ociSpec from grpc::Spec, which is needed for hook // execution. since hooks read config.json -use oci::{ - Box as ociBox, Hooks as ociHooks, Linux as ociLinux, LinuxCapabilities as ociLinuxCapabilities, - Mount as ociMount, POSIXRlimit as ociPOSIXRlimit, Process as ociProcess, Root as ociRoot, - Spec as ociSpec, User as ociUser, -}; -use protocols::oci::{ - Hooks as grpcHooks, Linux as grpcLinux, Mount as grpcMount, Process as grpcProcess, - Root as grpcRoot, Spec as grpcSpec, -}; use std::collections::HashMap; -pub fn process_grpc_to_oci(p: &grpcProcess) -> ociProcess { +pub fn process_grpc_to_oci(p: &grpc::Process) -> oci::Process { let console_size = if p.ConsoleSize.is_some() { let c = p.ConsoleSize.as_ref().unwrap(); - Some(ociBox { + Some(oci::Box { height: c.Height, width: c.Width, }) @@ -85,14 +78,14 @@ pub fn process_grpc_to_oci(p: &grpcProcess) -> ociProcess { let user = if p.User.is_some() { let u = p.User.as_ref().unwrap(); - ociUser { + oci::User { uid: u.UID, gid: u.GID, additional_gids: u.AdditionalGids.clone(), username: u.Username.clone(), } } else { - ociUser { + oci::User { uid: 0, gid: 0, additional_gids: vec![], @@ -103,7 +96,7 @@ pub fn process_grpc_to_oci(p: &grpcProcess) -> ociProcess { let capabilities = if p.Capabilities.is_some() { let cap = p.Capabilities.as_ref().unwrap(); - Some(ociLinuxCapabilities { + Some(oci::LinuxCapabilities { bounding: cap.Bounding.clone().into_vec(), effective: cap.Effective.clone().into_vec(), inheritable: cap.Inheritable.clone().into_vec(), @@ -117,7 +110,7 @@ pub fn process_grpc_to_oci(p: &grpcProcess) -> ociProcess { let rlimits = { let mut r = Vec::new(); for lm in p.Rlimits.iter() { - r.push(ociPOSIXRlimit { + r.push(oci::POSIXRlimit { r#type: lm.Type.clone(), hard: lm.Hard, soft: lm.Soft, @@ -126,7 +119,7 @@ pub fn process_grpc_to_oci(p: &grpcProcess) -> ociProcess { r }; - ociProcess { + oci::Process { terminal: p.Terminal, console_size, user, @@ -142,15 +135,15 @@ pub fn process_grpc_to_oci(p: &grpcProcess) -> ociProcess { } } -fn root_grpc_to_oci(root: &grpcRoot) -> ociRoot { - ociRoot { +fn root_grpc_to_oci(root: &grpc::Root) -> oci::Root { + oci::Root { path: root.Path.clone(), readonly: root.Readonly, } } -fn mount_grpc_to_oci(m: &grpcMount) -> ociMount { - ociMount { +fn mount_grpc_to_oci(m: &grpc::Mount) -> oci::Mount { + oci::Mount { destination: m.destination.clone(), r#type: m.field_type.clone(), source: m.source.clone(), @@ -158,13 +151,12 @@ fn mount_grpc_to_oci(m: &grpcMount) -> ociMount { } } -use oci::Hook as ociHook; use protocols::oci::Hook as grpcHook; -fn hook_grpc_to_oci(h: &[grpcHook]) -> Vec { +fn hook_grpc_to_oci(h: &[grpcHook]) -> Vec { let mut r = Vec::new(); for e in h.iter() { - r.push(ociHook { + r.push(oci::Hook { path: e.Path.clone(), args: e.Args.clone().into_vec(), env: e.Env.clone().into_vec(), @@ -174,39 +166,29 @@ fn hook_grpc_to_oci(h: &[grpcHook]) -> Vec { r } -fn hooks_grpc_to_oci(h: &grpcHooks) -> ociHooks { +fn hooks_grpc_to_oci(h: &grpc::Hooks) -> oci::Hooks { let prestart = hook_grpc_to_oci(h.Prestart.as_ref()); let poststart = hook_grpc_to_oci(h.Poststart.as_ref()); let poststop = hook_grpc_to_oci(h.Poststop.as_ref()); - ociHooks { + oci::Hooks { prestart, poststart, poststop, } } -use oci::{ - LinuxDevice as ociLinuxDevice, LinuxIDMapping as ociLinuxIDMapping, - LinuxIntelRdt as ociLinuxIntelRdt, LinuxNamespace as ociLinuxNamespace, - LinuxResources as ociLinuxResources, LinuxSeccomp as ociLinuxSeccomp, -}; -use protocols::oci::{ - LinuxIDMapping as grpcLinuxIDMapping, LinuxResources as grpcLinuxResources, - LinuxSeccomp as grpcLinuxSeccomp, -}; - -fn idmap_grpc_to_oci(im: &grpcLinuxIDMapping) -> ociLinuxIDMapping { - ociLinuxIDMapping { +fn idmap_grpc_to_oci(im: &grpc::LinuxIDMapping) -> oci::LinuxIDMapping { + oci::LinuxIDMapping { container_id: im.ContainerID, host_id: im.HostID, size: im.Size, } } -fn idmaps_grpc_to_oci(ims: &[grpcLinuxIDMapping]) -> Vec { +fn idmaps_grpc_to_oci(ims: &[grpc::LinuxIDMapping]) -> Vec { let mut r = Vec::new(); for im in ims.iter() { r.push(idmap_grpc_to_oci(im)); @@ -214,24 +196,13 @@ fn idmaps_grpc_to_oci(ims: &[grpcLinuxIDMapping]) -> Vec { r } -use oci::{ - LinuxBlockIO as ociLinuxBlockIO, LinuxBlockIODevice as ociLinuxBlockIODevice, - LinuxCPU as ociLinuxCPU, LinuxDeviceCgroup as ociLinuxDeviceCgroup, - LinuxHugepageLimit as ociLinuxHugepageLimit, - LinuxInterfacePriority as ociLinuxInterfacePriority, LinuxMemory as ociLinuxMemory, - LinuxNetwork as ociLinuxNetwork, LinuxPids as ociLinuxPids, - LinuxThrottleDevice as ociLinuxThrottleDevice, LinuxWeightDevice as ociLinuxWeightDevice, -}; -use protocols::oci::{ - LinuxBlockIO as grpcLinuxBlockIO, LinuxThrottleDevice as grpcLinuxThrottleDevice, - LinuxWeightDevice as grpcLinuxWeightDevice, -}; - -fn throttle_devices_grpc_to_oci(tds: &[grpcLinuxThrottleDevice]) -> Vec { +fn throttle_devices_grpc_to_oci( + tds: &[grpc::LinuxThrottleDevice], +) -> Vec { let mut r = Vec::new(); for td in tds.iter() { - r.push(ociLinuxThrottleDevice { - blk: ociLinuxBlockIODevice { + r.push(oci::LinuxThrottleDevice { + blk: oci::LinuxBlockIODevice { major: td.Major, minor: td.Minor, }, @@ -241,11 +212,11 @@ fn throttle_devices_grpc_to_oci(tds: &[grpcLinuxThrottleDevice]) -> Vec Vec { +fn weight_devices_grpc_to_oci(wds: &[grpc::LinuxWeightDevice]) -> Vec { let mut r = Vec::new(); for wd in wds.iter() { - r.push(ociLinuxWeightDevice { - blk: ociLinuxBlockIODevice { + r.push(oci::LinuxWeightDevice { + blk: oci::LinuxBlockIODevice { major: wd.Major, minor: wd.Minor, }, @@ -256,7 +227,7 @@ fn weight_devices_grpc_to_oci(wds: &[grpcLinuxWeightDevice]) -> Vec ociLinuxBlockIO { +fn blockio_grpc_to_oci(blk: &grpc::LinuxBlockIO) -> oci::LinuxBlockIO { let weight_device = weight_devices_grpc_to_oci(blk.WeightDevice.as_ref()); let throttle_read_bps_device = throttle_devices_grpc_to_oci(blk.ThrottleReadBpsDevice.as_ref()); let throttle_write_bps_device = @@ -266,7 +237,7 @@ fn blockio_grpc_to_oci(blk: &grpcLinuxBlockIO) -> ociLinuxBlockIO { let throttle_write_iops_device = throttle_devices_grpc_to_oci(blk.ThrottleWriteIOPSDevice.as_ref()); - ociLinuxBlockIO { + oci::LinuxBlockIO { weight: Some(blk.Weight as u16), leaf_weight: Some(blk.LeafWeight as u16), weight_device, @@ -277,7 +248,7 @@ fn blockio_grpc_to_oci(blk: &grpcLinuxBlockIO) -> ociLinuxBlockIO { } } -pub fn resources_grpc_to_oci(res: &grpcLinuxResources) -> ociLinuxResources { +pub fn resources_grpc_to_oci(res: &grpc::LinuxResources) -> oci::LinuxResources { let devices = { let mut d = Vec::new(); for dev in res.Devices.iter() { @@ -292,7 +263,7 @@ pub fn resources_grpc_to_oci(res: &grpcLinuxResources) -> ociLinuxResources { } else { Some(dev.Minor) }; - d.push(ociLinuxDeviceCgroup { + d.push(oci::LinuxDeviceCgroup { allow: dev.Allow, r#type: dev.Type.clone(), major, @@ -305,7 +276,7 @@ pub fn resources_grpc_to_oci(res: &grpcLinuxResources) -> ociLinuxResources { let memory = if res.Memory.is_some() { let mem = res.Memory.as_ref().unwrap(); - Some(ociLinuxMemory { + Some(oci::LinuxMemory { limit: Some(mem.Limit), reservation: Some(mem.Reservation), swap: Some(mem.Swap), @@ -320,7 +291,7 @@ pub fn resources_grpc_to_oci(res: &grpcLinuxResources) -> ociLinuxResources { let cpu = if res.CPU.is_some() { let c = res.CPU.as_ref().unwrap(); - Some(ociLinuxCPU { + Some(oci::LinuxCPU { shares: Some(c.Shares), quota: Some(c.Quota), period: Some(c.Period), @@ -335,7 +306,7 @@ pub fn resources_grpc_to_oci(res: &grpcLinuxResources) -> ociLinuxResources { let pids = if res.Pids.is_some() { let p = res.Pids.as_ref().unwrap(); - Some(ociLinuxPids { limit: p.Limit }) + Some(oci::LinuxPids { limit: p.Limit }) } else { None }; @@ -351,7 +322,7 @@ pub fn resources_grpc_to_oci(res: &grpcLinuxResources) -> ociLinuxResources { let hugepage_limits = { let mut r = Vec::new(); for hl in res.HugepageLimits.iter() { - r.push(ociLinuxHugepageLimit { + r.push(oci::LinuxHugepageLimit { page_size: hl.Pagesize.clone(), limit: hl.Limit, }); @@ -364,14 +335,14 @@ pub fn resources_grpc_to_oci(res: &grpcLinuxResources) -> ociLinuxResources { let priorities = { let mut r = Vec::new(); for pr in net.Priorities.iter() { - r.push(ociLinuxInterfacePriority { + r.push(oci::LinuxInterfacePriority { name: pr.Name.clone(), priority: pr.Priority, }); } r }; - Some(ociLinuxNetwork { + Some(oci::LinuxNetwork { class_id: Some(net.ClassID), priorities, }) @@ -379,7 +350,7 @@ pub fn resources_grpc_to_oci(res: &grpcLinuxResources) -> ociLinuxResources { None }; - ociLinuxResources { + oci::LinuxResources { devices, memory, cpu, @@ -391,9 +362,7 @@ pub fn resources_grpc_to_oci(res: &grpcLinuxResources) -> ociLinuxResources { } } -use oci::{LinuxSeccompArg as ociLinuxSeccompArg, LinuxSyscall as ociLinuxSyscall}; - -fn seccomp_grpc_to_oci(sec: &grpcLinuxSeccomp) -> ociLinuxSeccomp { +fn seccomp_grpc_to_oci(sec: &grpc::LinuxSeccomp) -> oci::LinuxSeccomp { let syscalls = { let mut r = Vec::new(); @@ -401,7 +370,7 @@ fn seccomp_grpc_to_oci(sec: &grpcLinuxSeccomp) -> ociLinuxSeccomp { let mut args = Vec::new(); for arg in sys.Args.iter() { - args.push(ociLinuxSeccompArg { + args.push(oci::LinuxSeccompArg { index: arg.Index as u32, value: arg.Value, value_two: arg.ValueTwo, @@ -409,7 +378,7 @@ fn seccomp_grpc_to_oci(sec: &grpcLinuxSeccomp) -> ociLinuxSeccomp { }); } - r.push(ociLinuxSyscall { + r.push(oci::LinuxSyscall { names: sys.Names.clone().into_vec(), action: sys.Action.clone(), errno_ret: sys.ErrnoRet, @@ -419,7 +388,7 @@ fn seccomp_grpc_to_oci(sec: &grpcLinuxSeccomp) -> ociLinuxSeccomp { r }; - ociLinuxSeccomp { + oci::LinuxSeccomp { default_action: sec.DefaultAction.clone(), architectures: sec.Architectures.clone().into_vec(), flags: sec.Flags.clone().into_vec(), @@ -427,7 +396,7 @@ fn seccomp_grpc_to_oci(sec: &grpcLinuxSeccomp) -> ociLinuxSeccomp { } } -fn linux_grpc_to_oci(l: &grpcLinux) -> ociLinux { +fn linux_grpc_to_oci(l: &grpc::Linux) -> oci::Linux { let uid_mappings = idmaps_grpc_to_oci(l.UIDMappings.as_ref()); let gid_mappings = idmaps_grpc_to_oci(l.GIDMappings.as_ref()); @@ -447,7 +416,7 @@ fn linux_grpc_to_oci(l: &grpcLinux) -> ociLinux { let mut r = Vec::new(); for ns in l.Namespaces.iter() { - r.push(ociLinuxNamespace { + r.push(oci::LinuxNamespace { r#type: ns.Type.clone(), path: ns.Path.clone(), }); @@ -459,7 +428,7 @@ fn linux_grpc_to_oci(l: &grpcLinux) -> ociLinux { let mut r = Vec::new(); for d in l.Devices.iter() { - r.push(ociLinuxDevice { + r.push(oci::LinuxDevice { path: d.Path.clone(), r#type: d.Type.clone(), major: d.Major, @@ -475,14 +444,14 @@ fn linux_grpc_to_oci(l: &grpcLinux) -> ociLinux { let intel_rdt = if l.IntelRdt.is_some() { let rdt = l.IntelRdt.as_ref().unwrap(); - Some(ociLinuxIntelRdt { + Some(oci::LinuxIntelRdt { l3_cache_schema: rdt.L3CacheSchema.clone(), }) } else { None }; - ociLinux { + oci::Linux { uid_mappings, gid_mappings, sysctl: l.Sysctl.clone(), @@ -499,11 +468,11 @@ fn linux_grpc_to_oci(l: &grpcLinux) -> ociLinux { } } -fn linux_oci_to_grpc(_l: &ociLinux) -> grpcLinux { - grpcLinux::default() +fn linux_oci_to_grpc(_l: &oci::Linux) -> grpc::Linux { + grpc::Linux::default() } -pub fn grpc_to_oci(grpc: &grpcSpec) -> ociSpec { +pub fn grpc_to_oci(grpc: &grpc::Spec) -> oci::Spec { // process let process = if grpc.Process.is_some() { Some(process_grpc_to_oci(grpc.Process.as_ref().unwrap())) @@ -541,7 +510,7 @@ pub fn grpc_to_oci(grpc: &grpcSpec) -> ociSpec { None }; - ociSpec { + oci::Spec { version: grpc.Version.clone(), process, root, From 3f5fdae0d841c83733dccfd20ec8f4ca1c7b2c45 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Thu, 22 Apr 2021 11:29:12 +1000 Subject: [PATCH 02/12] agent/rustjail: (trivial) Clean up comment on process_grpc_to_oci() This comment appears to be connected specifically with this function, but has some other items separating it for no particular reason. It also has a typo. Correct both. Signed-off-by: David Gibson --- src/agent/rustjail/src/lib.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/agent/rustjail/src/lib.rs b/src/agent/rustjail/src/lib.rs index a694f2390c..5416af5606 100644 --- a/src/agent/rustjail/src/lib.rs +++ b/src/agent/rustjail/src/lib.rs @@ -58,13 +58,12 @@ pub mod validator; // pub mod user; //pub mod intelrdt; -use protocols::oci as grpc; - -// construtc ociSpec from grpc::Spec, which is needed for hook -// execution. since hooks read config.json - use std::collections::HashMap; +use protocols::oci as grpc; + +// construct ociSpec from grpc::Spec, which is needed for hook +// execution. since hooks read config.json pub fn process_grpc_to_oci(p: &grpc::Process) -> oci::Process { let console_size = if p.ConsoleSize.is_some() { let c = p.ConsoleSize.as_ref().unwrap(); From eaec5a6c062bbb1e0656db707535790832fb29ec Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 14 Apr 2021 19:33:06 +1000 Subject: [PATCH 03/12] agent/oci: Change name case to make clippy happy Recent versions of clippy (e.g. in Rust 1.51) complain about a number of names in the oci crate, which don't obey Rust's normal CamelCasing conventions. It's pretty clear that these don't obey the usual rules because they are attempting to preserve conventional casing of existing acronyms they incorporate ("VM", "POSIX", etc.). However, it's been my experience that matching the case and name conventions of your environs is more important than matching case with external norms. Therefore, this patch changes all the identifiers in the oci crate to match Rust conventions. Their users in the rustjail crate are updated to match. fixes #1611 Signed-off-by: David Gibson --- src/agent/oci/src/lib.rs | 82 ++++++++++++------------ src/agent/rustjail/src/cgroups/fs/mod.rs | 6 +- src/agent/rustjail/src/container.rs | 46 ++++++------- src/agent/rustjail/src/lib.rs | 18 +++--- src/agent/rustjail/src/validator.rs | 14 ++-- 5 files changed, 83 insertions(+), 83 deletions(-) diff --git a/src/agent/oci/src/lib.rs b/src/agent/oci/src/lib.rs index 2e3c3f1109..69a2a98e15 100644 --- a/src/agent/oci/src/lib.rs +++ b/src/agent/oci/src/lib.rs @@ -58,7 +58,7 @@ pub struct Spec { #[serde(skip_serializing_if = "Option::is_none")] pub windows: Option>, #[serde(skip_serializing_if = "Option::is_none")] - pub vm: Option, + pub vm: Option, } impl Spec { @@ -71,7 +71,7 @@ impl Spec { } } -pub type LinuxRlimit = POSIXRlimit; +pub type LinuxRlimit = PosixRlimit; #[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq)] pub struct Process { @@ -93,7 +93,7 @@ pub struct Process { #[serde(default, skip_serializing_if = "Option::is_none")] pub capabilities: Option, #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub rlimits: Vec, + pub rlimits: Vec, #[serde(default, rename = "noNewPrivileges")] pub no_new_privileges: bool, #[serde( @@ -199,9 +199,9 @@ pub struct Hooks { #[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq)] pub struct Linux { #[serde(default, rename = "uidMappings", skip_serializing_if = "Vec::is_empty")] - pub uid_mappings: Vec, + pub uid_mappings: Vec, #[serde(default, rename = "gidMappings", skip_serializing_if = "Vec::is_empty")] - pub gid_mappings: Vec, + pub gid_mappings: Vec, #[serde(default, skip_serializing_if = "HashMap::is_empty")] pub sysctl: HashMap, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -261,7 +261,7 @@ pub const UTSNAMESPACE: &str = "uts"; pub const CGROUPNAMESPACE: &str = "cgroup"; #[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq)] -pub struct LinuxIDMapping { +pub struct LinuxIdMapping { #[serde(default, rename = "containerID")] pub container_id: u32, #[serde(default, rename = "hostID")] @@ -271,7 +271,7 @@ pub struct LinuxIDMapping { } #[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq)] -pub struct POSIXRlimit { +pub struct PosixRlimit { #[serde(default)] pub r#type: String, #[serde(default)] @@ -297,7 +297,7 @@ pub struct LinuxInterfacePriority { } #[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq)] -pub struct LinuxBlockIODevice { +pub struct LinuxBlockIoDevice { #[serde(default)] pub major: i64, #[serde(default)] @@ -307,7 +307,7 @@ pub struct LinuxBlockIODevice { #[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq)] pub struct LinuxWeightDevice { #[serde(flatten)] - pub blk: LinuxBlockIODevice, + pub blk: LinuxBlockIoDevice, #[serde(default, skip_serializing_if = "Option::is_none")] pub weight: Option, #[serde( @@ -321,13 +321,13 @@ pub struct LinuxWeightDevice { #[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq)] pub struct LinuxThrottleDevice { #[serde(flatten)] - pub blk: LinuxBlockIODevice, + pub blk: LinuxBlockIoDevice, #[serde(default)] pub rate: u64, } #[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq)] -pub struct LinuxBlockIO { +pub struct LinuxBlockIo { #[serde(default, skip_serializing_if = "Option::is_none")] pub weight: Option, #[serde( @@ -391,7 +391,7 @@ pub struct LinuxMemory { } #[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq)] -pub struct LinuxCPU { +pub struct LinuxCpu { #[serde(default, skip_serializing_if = "Option::is_none")] pub shares: Option, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -453,11 +453,11 @@ pub struct LinuxResources { #[serde(default, skip_serializing_if = "Option::is_none")] pub memory: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub cpu: Option, + pub cpu: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub pids: Option, #[serde(skip_serializing_if = "Option::is_none", rename = "blockIO")] - pub block_io: Option, + pub block_io: Option, #[serde( default, skip_serializing_if = "Vec::is_empty", @@ -517,7 +517,7 @@ pub struct Solaris { #[serde(default, skip_serializing_if = "Vec::is_empty")] pub anet: Vec, #[serde(default, skip_serializing_if = "Option::is_none", rename = "cappedCPU")] - pub capped_cpu: Option, + pub capped_cpu: Option, #[serde( default, skip_serializing_if = "Option::is_none", @@ -527,7 +527,7 @@ pub struct Solaris { } #[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq)] -pub struct SolarisCappedCPU { +pub struct SolarisCappedCpu { #[serde(default, skip_serializing_if = "String::is_empty")] pub ncpus: String, } @@ -605,7 +605,7 @@ pub struct WindowsResources { #[serde(default, skip_serializing_if = "Option::is_none")] pub memory: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub cpu: Option, + pub cpu: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub storage: Option, } @@ -617,7 +617,7 @@ pub struct WindowsMemoryResources { } #[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq)] -pub struct WindowsCPUResources { +pub struct WindowsCpuResources { #[serde(default, skip_serializing_if = "Option::is_none")] pub count: Option, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -675,14 +675,14 @@ pub struct WindowsHyperV { } #[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq)] -pub struct VM { - pub hypervisor: VMHypervisor, - pub kernel: VMKernel, - pub image: VMImage, +pub struct Vm { + pub hypervisor: VmHypervisor, + pub kernel: VmKernel, + pub image: VmImage, } #[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq)] -pub struct VMHypervisor { +pub struct VmHypervisor { #[serde(default)] pub path: String, #[serde(default, skip_serializing_if = "String::is_empty")] @@ -690,7 +690,7 @@ pub struct VMHypervisor { } #[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq)] -pub struct VMKernel { +pub struct VmKernel { #[serde(default)] pub path: String, #[serde(default, skip_serializing_if = "String::is_empty")] @@ -700,7 +700,7 @@ pub struct VMKernel { } #[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq)] -pub struct VMImage { +pub struct VmImage { #[serde(default)] pub path: String, #[serde(default)] @@ -801,11 +801,11 @@ pub struct LinuxIntelRdt { #[derive(Debug, Serialize, Deserialize, Copy, Clone, PartialEq)] #[serde(rename_all = "lowercase")] pub enum ContainerState { - CREATING, - CREATED, - RUNNING, - STOPPED, - PAUSED, + Creating, + Created, + Running, + Stopped, + Paused, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] @@ -846,7 +846,7 @@ mod tests { let expected = State { version: "0.2.0".to_string(), id: "oci-container1".to_string(), - status: ContainerState::RUNNING, + status: ContainerState::Running, pid: 4422, bundle: "/containers/redis".to_string(), annotations: [("myKey".to_string(), "myValue".to_string())] @@ -1271,12 +1271,12 @@ mod tests { ambient: vec!["CAP_NET_BIND_SERVICE".to_string()], }), rlimits: vec![ - crate::POSIXRlimit { + crate::PosixRlimit { r#type: "RLIMIT_CORE".to_string(), hard: 1024, soft: 1024, }, - crate::POSIXRlimit { + crate::PosixRlimit { r#type: "RLIMIT_NOFILE".to_string(), hard: 1024, soft: 1024, @@ -1408,12 +1408,12 @@ mod tests { .cloned() .collect(), linux: Some(crate::Linux { - uid_mappings: vec![crate::LinuxIDMapping { + uid_mappings: vec![crate::LinuxIdMapping { container_id: 0, host_id: 1000, size: 32000, }], - gid_mappings: vec![crate::LinuxIDMapping { + gid_mappings: vec![crate::LinuxIdMapping { container_id: 0, host_id: 1000, size: 32000, @@ -1458,7 +1458,7 @@ mod tests { swappiness: Some(0), disable_oom_killer: Some(false), }), - cpu: Some(crate::LinuxCPU { + cpu: Some(crate::LinuxCpu { shares: Some(1024), quota: Some(1000000), period: Some(500000), @@ -1468,17 +1468,17 @@ mod tests { mems: "0-7".to_string(), }), pids: Some(crate::LinuxPids { limit: 32771 }), - block_io: Some(crate::LinuxBlockIO { + block_io: Some(crate::LinuxBlockIo { weight: Some(10), leaf_weight: Some(10), weight_device: vec![ crate::LinuxWeightDevice { - blk: crate::LinuxBlockIODevice { major: 8, minor: 0 }, + blk: crate::LinuxBlockIoDevice { major: 8, minor: 0 }, weight: Some(500), leaf_weight: Some(300), }, crate::LinuxWeightDevice { - blk: crate::LinuxBlockIODevice { + blk: crate::LinuxBlockIoDevice { major: 8, minor: 16, }, @@ -1487,13 +1487,13 @@ mod tests { }, ], throttle_read_bps_device: vec![crate::LinuxThrottleDevice { - blk: crate::LinuxBlockIODevice { major: 8, minor: 0 }, + blk: crate::LinuxBlockIoDevice { major: 8, minor: 0 }, rate: 600, }], throttle_write_bps_device: vec![], throttle_read_iops_device: vec![], throttle_write_iops_device: vec![crate::LinuxThrottleDevice { - blk: crate::LinuxBlockIODevice { + blk: crate::LinuxBlockIoDevice { major: 8, minor: 16, }, diff --git a/src/agent/rustjail/src/cgroups/fs/mod.rs b/src/agent/rustjail/src/cgroups/fs/mod.rs index cc2e5dcffb..55aefed872 100644 --- a/src/agent/rustjail/src/cgroups/fs/mod.rs +++ b/src/agent/rustjail/src/cgroups/fs/mod.rs @@ -24,7 +24,7 @@ use anyhow::{anyhow, Context, Result}; use libc::{self, pid_t}; use nix::errno::Errno; use oci::{ - LinuxBlockIO, LinuxCPU, LinuxDevice, LinuxDeviceCgroup, LinuxHugepageLimit, LinuxMemory, + LinuxBlockIo, LinuxCpu, LinuxDevice, LinuxDeviceCgroup, LinuxHugepageLimit, LinuxMemory, LinuxNetwork, LinuxPids, LinuxResources, }; @@ -272,7 +272,7 @@ fn set_hugepages_resources( fn set_block_io_resources( _cg: &cgroups::Cgroup, - blkio: &LinuxBlockIO, + blkio: &LinuxBlockIo, res: &mut cgroups::Resources, ) { info!(sl!(), "cgroup manager set block io"); @@ -302,7 +302,7 @@ fn set_block_io_resources( build_blk_io_device_throttle_resource(&blkio.throttle_write_iops_device); } -fn set_cpu_resources(cg: &cgroups::Cgroup, cpu: &LinuxCPU) -> Result<()> { +fn set_cpu_resources(cg: &cgroups::Cgroup, cpu: &LinuxCpu) -> Result<()> { info!(sl!(), "cgroup manager set cpu"); let cpuset_controller: &CpuSetController = cg.controller_of().unwrap(); diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index 846257613e..0cfc5e75cf 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -5,7 +5,7 @@ use anyhow::{anyhow, Context, Result}; use libc::pid_t; -use oci::{ContainerState, LinuxDevice, LinuxIDMapping}; +use oci::{ContainerState, LinuxDevice, LinuxIdMapping}; use oci::{Hook, Linux, LinuxNamespace, LinuxResources, Spec}; use std::clone::Clone; use std::ffi::{CStr, CString}; @@ -83,8 +83,8 @@ pub struct ContainerStatus { impl ContainerStatus { fn new() -> Self { ContainerStatus { - pre_status: ContainerState::CREATED, - cur_status: ContainerState::CREATED, + pre_status: ContainerState::Created, + cur_status: ContainerState::Created, } } @@ -255,7 +255,7 @@ pub struct State { } #[derive(Serialize, Deserialize, Debug, Clone)] -pub struct SyncPC { +pub struct SyncPc { #[serde(default)] pid: pid_t, } @@ -268,7 +268,7 @@ pub trait Container: BaseContainer { impl Container for LinuxContainer { fn pause(&mut self) -> Result<()> { let status = self.status(); - if status != ContainerState::RUNNING && status != ContainerState::CREATED { + if status != ContainerState::Running && status != ContainerState::Created { return Err(anyhow!( "failed to pause container: current status is: {:?}", status @@ -281,7 +281,7 @@ impl Container for LinuxContainer { .unwrap() .freeze(FreezerState::Frozen)?; - self.status.transition(ContainerState::PAUSED); + self.status.transition(ContainerState::Paused); return Ok(()); } Err(anyhow!("failed to get container's cgroup manager")) @@ -289,7 +289,7 @@ impl Container for LinuxContainer { fn resume(&mut self) -> Result<()> { let status = self.status(); - if status != ContainerState::PAUSED { + if status != ContainerState::Paused { return Err(anyhow!("container status is: {:?}, not paused", status)); } @@ -299,7 +299,7 @@ impl Container for LinuxContainer { .unwrap() .freeze(FreezerState::Thawed)?; - self.status.transition(ContainerState::RUNNING); + self.status.transition(ContainerState::Running); return Ok(()); } Err(anyhow!("failed to get container's cgroup manager")) @@ -734,7 +734,7 @@ impl BaseContainer for LinuxContainer { }; let status = self.status(); - let pid = if status != ContainerState::STOPPED { + let pid = if status != ContainerState::Stopped { self.init_process_pid } else { 0 @@ -997,7 +997,7 @@ impl BaseContainer for LinuxContainer { if init { self.exec()?; - self.status.transition(ContainerState::RUNNING); + self.status.transition(ContainerState::Running); } Ok(()) @@ -1019,7 +1019,7 @@ impl BaseContainer for LinuxContainer { } } - self.status.transition(ContainerState::STOPPED); + self.status.transition(ContainerState::Stopped); mount::umount2( spec.root.as_ref().unwrap().path.as_str(), MntFlags::MNT_DETACH, @@ -1055,7 +1055,7 @@ impl BaseContainer for LinuxContainer { .unwrap() .as_secs(); - self.status.transition(ContainerState::RUNNING); + self.status.transition(ContainerState::Running); unistd::close(fd)?; Ok(()) @@ -1302,7 +1302,7 @@ async fn join_namespaces( Ok(()) } -fn write_mappings(logger: &Logger, path: &str, maps: &[LinuxIDMapping]) -> Result<()> { +fn write_mappings(logger: &Logger, path: &str, maps: &[LinuxIdMapping]) -> Result<()> { let data = maps .iter() .filter(|m| m.size != 0) @@ -1588,7 +1588,7 @@ mod tests { &OCIState { version: "1.2.3".to_string(), id: "321".to_string(), - status: ContainerState::RUNNING, + status: ContainerState::Running, pid: 2, bundle: "".to_string(), annotations: Default::default(), @@ -1611,7 +1611,7 @@ mod tests { &OCIState { version: "1.2.3".to_string(), id: "321".to_string(), - status: ContainerState::RUNNING, + status: ContainerState::Running, pid: 2, bundle: "".to_string(), annotations: Default::default(), @@ -1630,10 +1630,10 @@ mod tests { fn test_status_transtition() { let mut status = ContainerStatus::new(); let status_table: [ContainerState; 4] = [ - ContainerState::CREATED, - ContainerState::RUNNING, - ContainerState::PAUSED, - ContainerState::STOPPED, + ContainerState::Created, + ContainerState::Running, + ContainerState::Paused, + ContainerState::Stopped, ]; for s in status_table.iter() { @@ -1770,7 +1770,7 @@ mod tests { fn test_linuxcontainer_pause_bad_status() { let ret = new_linux_container_and_then(|mut c: LinuxContainer| { // Change state to pause, c.pause() should fail - c.status.transition(ContainerState::PAUSED); + c.status.transition(ContainerState::Paused); c.pause().map_err(|e| anyhow!(e)) }); @@ -1802,7 +1802,7 @@ mod tests { fn test_linuxcontainer_resume_bad_status() { let ret = new_linux_container_and_then(|mut c: LinuxContainer| { // Change state to created, c.resume() should fail - c.status.transition(ContainerState::CREATED); + c.status.transition(ContainerState::Created); c.resume().map_err(|e| anyhow!(e)) }); @@ -1813,7 +1813,7 @@ mod tests { #[test] fn test_linuxcontainer_resume_cgroupmgr_is_none() { let ret = new_linux_container_and_then(|mut c: LinuxContainer| { - c.status.transition(ContainerState::PAUSED); + c.status.transition(ContainerState::Paused); c.cgroup_manager = None; c.resume().map_err(|e| anyhow!(e)) }); @@ -1826,7 +1826,7 @@ mod tests { let ret = new_linux_container_and_then(|mut c: LinuxContainer| { c.cgroup_manager = FsManager::new("").ok(); // Change status to paused, this way we can resume it - c.status.transition(ContainerState::PAUSED); + c.status.transition(ContainerState::Paused); c.resume().map_err(|e| anyhow!(e)) }); diff --git a/src/agent/rustjail/src/lib.rs b/src/agent/rustjail/src/lib.rs index 5416af5606..0d4f508653 100644 --- a/src/agent/rustjail/src/lib.rs +++ b/src/agent/rustjail/src/lib.rs @@ -109,7 +109,7 @@ pub fn process_grpc_to_oci(p: &grpc::Process) -> oci::Process { let rlimits = { let mut r = Vec::new(); for lm in p.Rlimits.iter() { - r.push(oci::POSIXRlimit { + r.push(oci::PosixRlimit { r#type: lm.Type.clone(), hard: lm.Hard, soft: lm.Soft, @@ -179,15 +179,15 @@ fn hooks_grpc_to_oci(h: &grpc::Hooks) -> oci::Hooks { } } -fn idmap_grpc_to_oci(im: &grpc::LinuxIDMapping) -> oci::LinuxIDMapping { - oci::LinuxIDMapping { +fn idmap_grpc_to_oci(im: &grpc::LinuxIDMapping) -> oci::LinuxIdMapping { + oci::LinuxIdMapping { container_id: im.ContainerID, host_id: im.HostID, size: im.Size, } } -fn idmaps_grpc_to_oci(ims: &[grpc::LinuxIDMapping]) -> Vec { +fn idmaps_grpc_to_oci(ims: &[grpc::LinuxIDMapping]) -> Vec { let mut r = Vec::new(); for im in ims.iter() { r.push(idmap_grpc_to_oci(im)); @@ -201,7 +201,7 @@ fn throttle_devices_grpc_to_oci( let mut r = Vec::new(); for td in tds.iter() { r.push(oci::LinuxThrottleDevice { - blk: oci::LinuxBlockIODevice { + blk: oci::LinuxBlockIoDevice { major: td.Major, minor: td.Minor, }, @@ -215,7 +215,7 @@ fn weight_devices_grpc_to_oci(wds: &[grpc::LinuxWeightDevice]) -> Vec Vec oci::LinuxBlockIO { +fn blockio_grpc_to_oci(blk: &grpc::LinuxBlockIO) -> oci::LinuxBlockIo { let weight_device = weight_devices_grpc_to_oci(blk.WeightDevice.as_ref()); let throttle_read_bps_device = throttle_devices_grpc_to_oci(blk.ThrottleReadBpsDevice.as_ref()); let throttle_write_bps_device = @@ -236,7 +236,7 @@ fn blockio_grpc_to_oci(blk: &grpc::LinuxBlockIO) -> oci::LinuxBlockIO { let throttle_write_iops_device = throttle_devices_grpc_to_oci(blk.ThrottleWriteIOPSDevice.as_ref()); - oci::LinuxBlockIO { + oci::LinuxBlockIo { weight: Some(blk.Weight as u16), leaf_weight: Some(blk.LeafWeight as u16), weight_device, @@ -290,7 +290,7 @@ pub fn resources_grpc_to_oci(res: &grpc::LinuxResources) -> oci::LinuxResources let cpu = if res.CPU.is_some() { let c = res.CPU.as_ref().unwrap(); - Some(oci::LinuxCPU { + Some(oci::LinuxCpu { shares: Some(c.Shares), quota: Some(c.Quota), period: Some(c.Period), diff --git a/src/agent/rustjail/src/validator.rs b/src/agent/rustjail/src/validator.rs index b5fabf9296..9dbbd19ddb 100644 --- a/src/agent/rustjail/src/validator.rs +++ b/src/agent/rustjail/src/validator.rs @@ -6,7 +6,7 @@ use crate::container::Config; use anyhow::{anyhow, Context, Error, Result}; use nix::errno::Errno; -use oci::{Linux, LinuxIDMapping, LinuxNamespace, Spec}; +use oci::{Linux, LinuxIdMapping, LinuxNamespace, Spec}; use std::collections::HashMap; use std::path::{Component, PathBuf}; @@ -107,7 +107,7 @@ fn security(oci: &Spec) -> Result<()> { Ok(()) } -fn idmapping(maps: &[LinuxIDMapping]) -> Result<()> { +fn idmapping(maps: &[LinuxIdMapping]) -> Result<()> { for map in maps { if map.size > 0 { return Ok(()); @@ -238,7 +238,7 @@ fn rootless_euid_mapping(oci: &Spec) -> Result<()> { Ok(()) } -fn has_idmapping(maps: &[LinuxIDMapping], id: u32) -> bool { +fn has_idmapping(maps: &[LinuxIdMapping], id: u32) -> bool { for map in maps { if id >= map.container_id && id < map.container_id + map.size { return true; @@ -441,7 +441,7 @@ mod tests { usernamespace(&spec).unwrap(); let mut linux = Linux::default(); - linux.uid_mappings = vec![LinuxIDMapping { + linux.uid_mappings = vec![LinuxIdMapping { container_id: 0, host_id: 1000, size: 0, @@ -450,7 +450,7 @@ mod tests { usernamespace(&spec).unwrap_err(); let mut linux = Linux::default(); - linux.uid_mappings = vec![LinuxIDMapping { + linux.uid_mappings = vec![LinuxIdMapping { container_id: 0, host_id: 1000, size: 100, @@ -497,12 +497,12 @@ mod tests { path: "/sys/cgroups/user".to_owned(), }, ]; - linux.uid_mappings = vec![LinuxIDMapping { + linux.uid_mappings = vec![LinuxIdMapping { container_id: 0, host_id: 1000, size: 1000, }]; - linux.gid_mappings = vec![LinuxIDMapping { + linux.gid_mappings = vec![LinuxIdMapping { container_id: 0, host_id: 1000, size: 1000, From 3c4485ece37d1de95060963fde5c74c12564f5eb Mon Sep 17 00:00:00 2001 From: David Gibson Date: Fri, 16 Apr 2021 16:26:28 +1000 Subject: [PATCH 04/12] agent/rustjail: Clean up some static definitions with vec! macro DEFAULT_ALLOWED_DEVICES and DEFAULT_DEVICES are essentially global constant lists. They're implemented as a lazy_static! initialized Vec values. The code to initialize them creates an empty Vec then pushes values onto it. We can simplify this a bit by using the vec! macro. This might be slightly more efficient, and it definitely stops recent clippy versions (e.g. 1.51) from complaining about it. fixes #1611 Signed-off-by: David Gibson --- src/agent/rustjail/src/cgroups/fs/mod.rs | 102 ++++++++++----------- src/agent/rustjail/src/container.rs | 112 +++++++++++------------ 2 files changed, 106 insertions(+), 108 deletions(-) diff --git a/src/agent/rustjail/src/cgroups/fs/mod.rs b/src/agent/rustjail/src/cgroups/fs/mod.rs index 55aefed872..7f41cb4ddb 100644 --- a/src/agent/rustjail/src/cgroups/fs/mod.rs +++ b/src/agent/rustjail/src/cgroups/fs/mod.rs @@ -489,63 +489,61 @@ lazy_static! { }; pub static ref DEFAULT_ALLOWED_DEVICES: Vec = { - let mut v = Vec::new(); + vec![ + // all mknod to all char devices + LinuxDeviceCgroup { + allow: true, + r#type: "c".to_string(), + major: Some(WILDCARD), + minor: Some(WILDCARD), + access: "m".to_string(), + }, - // all mknod to all char devices - v.push(LinuxDeviceCgroup { - allow: true, - r#type: "c".to_string(), - major: Some(WILDCARD), - minor: Some(WILDCARD), - access: "m".to_string(), - }); + // all mknod to all block devices + LinuxDeviceCgroup { + allow: true, + r#type: "b".to_string(), + major: Some(WILDCARD), + minor: Some(WILDCARD), + access: "m".to_string(), + }, - // all mknod to all block devices - v.push(LinuxDeviceCgroup { - allow: true, - r#type: "b".to_string(), - major: Some(WILDCARD), - minor: Some(WILDCARD), - access: "m".to_string(), - }); + // all read/write/mknod to char device /dev/console + LinuxDeviceCgroup { + allow: true, + r#type: "c".to_string(), + major: Some(5), + minor: Some(1), + access: "rwm".to_string(), + }, - // all read/write/mknod to char device /dev/console - v.push(LinuxDeviceCgroup { - allow: true, - r#type: "c".to_string(), - major: Some(5), - minor: Some(1), - access: "rwm".to_string(), - }); + // all read/write/mknod to char device /dev/pts/ + LinuxDeviceCgroup { + allow: true, + r#type: "c".to_string(), + major: Some(136), + minor: Some(WILDCARD), + access: "rwm".to_string(), + }, - // all read/write/mknod to char device /dev/pts/ - v.push(LinuxDeviceCgroup { - allow: true, - r#type: "c".to_string(), - major: Some(136), - minor: Some(WILDCARD), - access: "rwm".to_string(), - }); + // all read/write/mknod to char device /dev/ptmx + LinuxDeviceCgroup { + allow: true, + r#type: "c".to_string(), + major: Some(5), + minor: Some(2), + access: "rwm".to_string(), + }, - // all read/write/mknod to char device /dev/ptmx - v.push(LinuxDeviceCgroup { - allow: true, - r#type: "c".to_string(), - major: Some(5), - minor: Some(2), - access: "rwm".to_string(), - }); - - // all read/write/mknod to char device /dev/net/tun - v.push(LinuxDeviceCgroup { - allow: true, - r#type: "c".to_string(), - major: Some(10), - minor: Some(200), - access: "rwm".to_string(), - }); - - v + // all read/write/mknod to char device /dev/net/tun + LinuxDeviceCgroup { + allow: true, + r#type: "c".to_string(), + major: Some(10), + minor: Some(200), + access: "rwm".to_string(), + }, + ] }; } diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index 0cfc5e75cf..25be015e06 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -132,62 +132,62 @@ lazy_static! { }; pub static ref DEFAULT_DEVICES: Vec = { - let mut v = Vec::new(); - v.push(LinuxDevice { - path: "/dev/null".to_string(), - r#type: "c".to_string(), - major: 1, - minor: 3, - file_mode: Some(0o666), - uid: Some(0xffffffff), - gid: Some(0xffffffff), - }); - v.push(LinuxDevice { - path: "/dev/zero".to_string(), - r#type: "c".to_string(), - major: 1, - minor: 5, - file_mode: Some(0o666), - uid: Some(0xffffffff), - gid: Some(0xffffffff), - }); - v.push(LinuxDevice { - path: "/dev/full".to_string(), - r#type: String::from("c"), - major: 1, - minor: 7, - file_mode: Some(0o666), - uid: Some(0xffffffff), - gid: Some(0xffffffff), - }); - v.push(LinuxDevice { - path: "/dev/tty".to_string(), - r#type: "c".to_string(), - major: 5, - minor: 0, - file_mode: Some(0o666), - uid: Some(0xffffffff), - gid: Some(0xffffffff), - }); - v.push(LinuxDevice { - path: "/dev/urandom".to_string(), - r#type: "c".to_string(), - major: 1, - minor: 9, - file_mode: Some(0o666), - uid: Some(0xffffffff), - gid: Some(0xffffffff), - }); - v.push(LinuxDevice { - path: "/dev/random".to_string(), - r#type: "c".to_string(), - major: 1, - minor: 8, - file_mode: Some(0o666), - uid: Some(0xffffffff), - gid: Some(0xffffffff), - }); - v + vec![ + LinuxDevice { + path: "/dev/null".to_string(), + r#type: "c".to_string(), + major: 1, + minor: 3, + file_mode: Some(0o666), + uid: Some(0xffffffff), + gid: Some(0xffffffff), + }, + LinuxDevice { + path: "/dev/zero".to_string(), + r#type: "c".to_string(), + major: 1, + minor: 5, + file_mode: Some(0o666), + uid: Some(0xffffffff), + gid: Some(0xffffffff), + }, + LinuxDevice { + path: "/dev/full".to_string(), + r#type: String::from("c"), + major: 1, + minor: 7, + file_mode: Some(0o666), + uid: Some(0xffffffff), + gid: Some(0xffffffff), + }, + LinuxDevice { + path: "/dev/tty".to_string(), + r#type: "c".to_string(), + major: 5, + minor: 0, + file_mode: Some(0o666), + uid: Some(0xffffffff), + gid: Some(0xffffffff), + }, + LinuxDevice { + path: "/dev/urandom".to_string(), + r#type: "c".to_string(), + major: 1, + minor: 9, + file_mode: Some(0o666), + uid: Some(0xffffffff), + gid: Some(0xffffffff), + }, + LinuxDevice { + path: "/dev/random".to_string(), + r#type: "c".to_string(), + major: 1, + minor: 8, + file_mode: Some(0o666), + uid: Some(0xffffffff), + gid: Some(0xffffffff), + }, + ] }; } From 6ce1e56d20adf36e3ac62a829caf9d10cbb1a52a Mon Sep 17 00:00:00 2001 From: David Gibson Date: Fri, 16 Apr 2021 16:28:29 +1000 Subject: [PATCH 05/12] agent/rustjail: Remove an unnecessary PathBuf PathBuf is an owned, mutable Path. We don't need those properties in get_value_from_cgroup() so we can use a Path instead. This may be slightly safer, and definitely stops clippy (version 1.51 at least) from complaining. fixes #1611 Signed-off-by: David Gibson --- src/agent/rustjail/src/cgroups/notifier.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/agent/rustjail/src/cgroups/notifier.rs b/src/agent/rustjail/src/cgroups/notifier.rs index 91c88be1f9..a3a7f4e855 100644 --- a/src/agent/rustjail/src/cgroups/notifier.rs +++ b/src/agent/rustjail/src/cgroups/notifier.rs @@ -8,7 +8,7 @@ use eventfd::{eventfd, EfdFlags}; use nix::sys::eventfd; use std::fs::{self, File}; use std::os::unix::io::{AsRawFd, FromRawFd}; -use std::path::{Path, PathBuf}; +use std::path::Path; use crate::pipestream::PipeStream; use futures::StreamExt as _; @@ -35,7 +35,7 @@ pub async fn notify_oom(cid: &str, cg_dir: String) -> Result> { // Flat keyed file format: // KEY0 VAL0\n // KEY1 VAL1\n -fn get_value_from_cgroup(path: &PathBuf, key: &str) -> Result { +fn get_value_from_cgroup(path: &Path, key: &str) -> Result { let content = fs::read_to_string(path)?; info!( sl!(), From 75eca6d56f1540b93452c0417547db57291178e6 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Fri, 16 Apr 2021 16:41:13 +1000 Subject: [PATCH 06/12] agent/rustjail: Clean up error path in execute_hook()s async task Clippy (in Rust 1.51 at least) has some complaints about this closure inside execute_hook() because it uses explicit returns in some places where it doesn't need them, because they're the last expression in the function. That isn't necessarily obvious from a glance, but we can make clippy happy and also make things a little clearer: first we replace a somewhat verbose 'match' using Option::ok_or_else(), then rearrange the remaining code to put all the error path first with an explicit return then the "happy" path as the stright line exit with an implicit return. fixes #1611 Signed-off-by: David Gibson --- src/agent/rustjail/src/container.rs | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index 25be015e06..ede65926aa 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -1528,28 +1528,23 @@ async fn execute_hook(logger: &Logger, h: &Hook, st: &OCIState) -> Result<()> { match child.wait().await { Ok(exit) => { - let code = match exit.code() { - Some(c) => c, - None => { - return Err(anyhow!("hook exit status has no status code")); - } - }; + let code = exit + .code() + .ok_or_else(|| anyhow!("hook exit status has no status code"))?; - if code == 0 { - debug!(logger, "hook {} exit status is 0", &path); - return Ok(()); - } else { + if code != 0 { error!(logger, "hook {} exit status is {}", &path, code); return Err(anyhow!(nix::Error::from_errno(Errno::UnknownErrno))); } + + debug!(logger, "hook {} exit status is 0", &path); + Ok(()) } - Err(e) => { - return Err(anyhow!( - "wait child error: {} {}", - e, - e.raw_os_error().unwrap() - )); - } + Err(e) => Err(anyhow!( + "wait child error: {} {}", + e, + e.raw_os_error().unwrap() + )), } }); From 2377c0975c0a1e04c1c94d65378821bd29ff5caa Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 21 Apr 2021 14:16:11 +1000 Subject: [PATCH 07/12] agent: Use CamelCase for NamespaceType values Currently these are in all-caps, to match typical capitalization of IPC, UTS and PID in the world at large. However, this violates Rust's capitalization conventions and makes clippy complain. fixes #1611 Signed-off-by: David Gibson --- src/agent/src/namespace.rs | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/agent/src/namespace.rs b/src/agent/src/namespace.rs index 6ae22e12fa..20dd2d9596 100644 --- a/src/agent/src/namespace.rs +++ b/src/agent/src/namespace.rs @@ -45,18 +45,18 @@ impl Namespace { logger: logger.clone(), path: String::from(""), persistent_ns_dir: String::from(PERSISTENT_NS_DIR), - ns_type: NamespaceType::IPC, + ns_type: NamespaceType::Ipc, hostname: None, } } pub fn get_ipc(mut self) -> Self { - self.ns_type = NamespaceType::IPC; + self.ns_type = NamespaceType::Ipc; self } pub fn get_uts(mut self, hostname: &str) -> Self { - self.ns_type = NamespaceType::UTS; + self.ns_type = NamespaceType::Uts; if !hostname.is_empty() { self.hostname = Some(String::from(hostname)); } @@ -64,7 +64,7 @@ impl Namespace { } pub fn get_pid(mut self) -> Self { - self.ns_type = NamespaceType::PID; + self.ns_type = NamespaceType::Pid; self } @@ -81,7 +81,7 @@ impl Namespace { let ns_path = PathBuf::from(&self.persistent_ns_dir); let ns_type = self.ns_type; - if ns_type == NamespaceType::PID { + if ns_type == NamespaceType::Pid { return Err(anyhow!("Cannot persist namespace of PID type")); } let logger = self.logger.clone(); @@ -104,7 +104,7 @@ impl Namespace { unshare(cf)?; - if ns_type == NamespaceType::UTS && hostname.is_some() { + if ns_type == NamespaceType::Uts && hostname.is_some() { nix::unistd::sethostname(hostname.unwrap())?; } // Bind mount the new namespace from the current thread onto the mount point to persist it. @@ -147,27 +147,27 @@ impl Namespace { /// Represents the Namespace type. #[derive(Clone, Copy, PartialEq)] enum NamespaceType { - IPC, - UTS, - PID, + Ipc, + Uts, + Pid, } impl NamespaceType { /// Get the string representation of the namespace type. pub fn get(&self) -> &str { match *self { - Self::IPC => "ipc", - Self::UTS => "uts", - Self::PID => "pid", + Self::Ipc => "ipc", + Self::Uts => "uts", + Self::Pid => "pid", } } /// Get the associate flags with the namespace type. pub fn get_flags(&self) -> CloneFlags { match *self { - Self::IPC => CloneFlags::CLONE_NEWIPC, - Self::UTS => CloneFlags::CLONE_NEWUTS, - Self::PID => CloneFlags::CLONE_NEWPID, + Self::Ipc => CloneFlags::CLONE_NEWIPC, + Self::Uts => CloneFlags::CLONE_NEWUTS, + Self::Pid => CloneFlags::CLONE_NEWPID, } } } @@ -180,7 +180,7 @@ impl fmt::Debug for NamespaceType { impl Default for NamespaceType { fn default() -> Self { - NamespaceType::IPC + NamespaceType::Ipc } } @@ -234,15 +234,15 @@ mod tests { #[test] fn test_namespace_type() { - let ipc = NamespaceType::IPC; + let ipc = NamespaceType::Ipc; assert_eq!("ipc", ipc.get()); assert_eq!(CloneFlags::CLONE_NEWIPC, ipc.get_flags()); - let uts = NamespaceType::UTS; + let uts = NamespaceType::Uts; assert_eq!("uts", uts.get()); assert_eq!(CloneFlags::CLONE_NEWUTS, uts.get_flags()); - let pid = NamespaceType::PID; + let pid = NamespaceType::Pid; assert_eq!("pid", pid.get()); assert_eq!(CloneFlags::CLONE_NEWPID, pid.get_flags()); } From e41cdb8b9f792626b7a6aaaea521fbc9d865f994 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 21 Apr 2021 14:18:42 +1000 Subject: [PATCH 08/12] agent: Use str::is_empty() method in config::get_string_value() An explicit check against "" is a bit less clear and makes clippy complain. fixes #1611 Signed-off-by: David Gibson --- src/agent/src/config.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/agent/src/config.rs b/src/agent/src/config.rs index 3d6e168e62..bc415cdc15 100644 --- a/src/agent/src/config.rs +++ b/src/agent/src/config.rs @@ -273,12 +273,12 @@ fn get_string_value(param: &str) -> Result { } // We need name (but the value can be blank) - if fields[0] == "" { + if fields[0].is_empty() { return Err(anyhow!(ERR_INVALID_GET_VALUE_NO_NAME)); } let value = fields[1..].join("="); - if value == "" { + if value.is_empty() { return Err(anyhow!(ERR_INVALID_GET_VALUE_NO_VALUE)); } From 1c43245e3e62cd1c20555e18dfb307d1e2001bdb Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 21 Apr 2021 14:21:56 +1000 Subject: [PATCH 09/12] agent/device: Remove unneeded Result<> wrappers from uev matchers The various type implementing the UeventMatcher trait have new() methods which return a Result<>, however none of them can actually fail. This is a leftover from their development where some versions could fail to initialize. Remove the unneccessary wrappers to silence clippy. fixes #1611 Signed-off-by: David Gibson --- src/agent/src/device.rs | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/agent/src/device.rs b/src/agent/src/device.rs index 45c969a690..29efb07997 100644 --- a/src/agent/src/device.rs +++ b/src/agent/src/device.rs @@ -96,10 +96,10 @@ struct ScsiBlockMatcher { } impl ScsiBlockMatcher { - fn new(scsi_addr: &str) -> Result { + fn new(scsi_addr: &str) -> ScsiBlockMatcher { let search = format!(r"/0:0:{}/block/", scsi_addr); - Ok(ScsiBlockMatcher { search }) + ScsiBlockMatcher { search } } } @@ -113,7 +113,7 @@ pub async fn get_scsi_device_name( sandbox: &Arc>, scsi_addr: &str, ) -> Result { - let matcher = ScsiBlockMatcher::new(scsi_addr)?; + let matcher = ScsiBlockMatcher::new(scsi_addr); scan_scsi_bus(scsi_addr)?; let uev = wait_for_uevent(sandbox, matcher).await?; @@ -126,12 +126,12 @@ struct VirtioBlkPciMatcher { } impl VirtioBlkPciMatcher { - fn new(relpath: &str) -> Result { + fn new(relpath: &str) -> VirtioBlkPciMatcher { let root_bus = create_pci_root_bus_path(); let re = format!(r"^{}{}/virtio[0-9]+/block/", root_bus, relpath); - Ok(VirtioBlkPciMatcher { + VirtioBlkPciMatcher { rex: Regex::new(&re).unwrap(), - }) + } } } @@ -147,7 +147,7 @@ pub async fn get_virtio_blk_pci_device_name( ) -> Result { let root_bus_sysfs = format!("{}{}", SYSFS_DIR, create_pci_root_bus_path()); let sysfs_rel_path = pcipath_to_sysfs(&root_bus_sysfs, pcipath)?; - let matcher = VirtioBlkPciMatcher::new(&sysfs_rel_path)?; + let matcher = VirtioBlkPciMatcher::new(&sysfs_rel_path); rescan_pci_bus()?; @@ -161,10 +161,10 @@ struct PmemBlockMatcher { } impl PmemBlockMatcher { - fn new(devname: &str) -> Result { + fn new(devname: &str) -> PmemBlockMatcher { let suffix = format!(r"/block/{}", devname); - Ok(PmemBlockMatcher { suffix }) + PmemBlockMatcher { suffix } } } @@ -188,7 +188,7 @@ pub async fn wait_for_pmem_device(sandbox: &Arc>, devpath: &str) } }; - let matcher = PmemBlockMatcher::new(devname)?; + let matcher = PmemBlockMatcher::new(devname); let uev = wait_for_uevent(sandbox, matcher).await?; if uev.devname != devname { return Err(anyhow!( @@ -822,7 +822,7 @@ mod tests { sandbox: &Arc>, relpath: &str, ) -> Result { - let matcher = VirtioBlkPciMatcher::new(relpath)?; + let matcher = VirtioBlkPciMatcher::new(relpath); let uev = wait_for_uevent(sandbox, matcher).await?; @@ -875,12 +875,12 @@ mod tests { uev_a.subsystem = "block".to_string(); uev_a.devname = devname.to_string(); uev_a.devpath = format!("{}{}/virtio4/block/{}", root_bus, relpath_a, devname); - let matcher_a = VirtioBlkPciMatcher::new(&relpath_a).unwrap(); + let matcher_a = VirtioBlkPciMatcher::new(&relpath_a); let mut uev_b = uev_a.clone(); let relpath_b = "/0000:00:0a.0/0000:00:0b.0"; uev_b.devpath = format!("{}{}/virtio0/block/{}", root_bus, relpath_b, devname); - let matcher_b = VirtioBlkPciMatcher::new(&relpath_b).unwrap(); + let matcher_b = VirtioBlkPciMatcher::new(&relpath_b); assert!(matcher_a.is_match(&uev_a)); assert!(matcher_b.is_match(&uev_b)); @@ -902,7 +902,7 @@ mod tests { "{}/0000:00:00.0/virtio0/host0/target0:0:0/0:0:{}/block/sda", root_bus, addr_a ); - let matcher_a = ScsiBlockMatcher::new(&addr_a).unwrap(); + let matcher_a = ScsiBlockMatcher::new(&addr_a); let mut uev_b = uev_a.clone(); let addr_b = "2:0"; @@ -910,7 +910,7 @@ mod tests { "{}/0000:00:00.0/virtio0/host0/target0:0:2/0:0:{}/block/sdb", root_bus, addr_b ); - let matcher_b = ScsiBlockMatcher::new(&addr_b).unwrap(); + let matcher_b = ScsiBlockMatcher::new(&addr_b); assert!(matcher_a.is_match(&uev_a)); assert!(matcher_b.is_match(&uev_b)); From b0190a407fe82d2c5f78fc2be65c14d13fc35e33 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 21 Apr 2021 14:23:23 +1000 Subject: [PATCH 10/12] agent: Use vec![] macro rather than init-then-push We have one place where we create an empty vector then immediately push something into it. We can do this in one step using the vec![] macro, which stops clippy complaining. fixes #1611 Signed-off-by: David Gibson --- src/agent/src/netlink.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/agent/src/netlink.rs b/src/agent/src/netlink.rs index 5598a93865..3ab6dbaa0d 100644 --- a/src/agent/src/netlink.rs +++ b/src/agent/src/netlink.rs @@ -542,12 +542,10 @@ impl Handle { ntype: NDA_UNSPEC as u8, }, nlas: { - let mut nlas = vec![]; - - nlas.push(Nla::Destination(match ip { + let mut nlas = vec![Nla::Destination(match ip { IpAddr::V4(v4) => v4.octets().to_vec(), IpAddr::V6(v6) => v6.octets().to_vec(), - })); + })]; if !neigh.lladdr.is_empty() { nlas.push(Nla::LinkLocalAddress( From 7b83b7ec1f07d4545b653d24d847cd9e0e9965c5 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 21 Apr 2021 14:25:55 +1000 Subject: [PATCH 11/12] agent/uevent: Better initialize Uevent in test We had some code that initialized a Uevent to the default value, then set specific fields to various values. This can be accomplished inside the one initialized using the ..Default::default() syntax. Making this change stops clippy from complaining. fixes #1611 Signed-off-by: David Gibson --- src/agent/src/uevent.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/agent/src/uevent.rs b/src/agent/src/uevent.rs index 3d8e12b0d5..d00de30160 100644 --- a/src/agent/src/uevent.rs +++ b/src/agent/src/uevent.rs @@ -245,11 +245,13 @@ mod tests { #[tokio::test] async fn test_wait_for_uevent() { - let mut uev = crate::uevent::Uevent::default(); - uev.action = crate::linux_abi::U_EVENT_ACTION_ADD.to_string(); - uev.subsystem = "test".to_string(); - uev.devpath = "/test/sysfs/path".to_string(); - uev.devname = "testdevname".to_string(); + let uev = Uevent { + action: crate::linux_abi::U_EVENT_ACTION_ADD.to_string(), + subsystem: "test".to_string(), + devpath: "/test/sysfs/path".to_string(), + devname: "testdevname".to_string(), + ..Default::default() + }; let matcher = AlwaysMatch(); From 0405beb2d82148c25d363491afc74b05dc0e124c Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 21 Apr 2021 14:50:06 +1000 Subject: [PATCH 12/12] agent: Remove unused Default implementation for NamespaceType Currently we implement the Default trait for NamespaceType. It doesn't really make sense to have a default for this type though - you really need to know what type of namespace you're setting. In fact the Default implementation is never used, so we can just drop it. Signed-off-by: David Gibson --- src/agent/src/namespace.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/agent/src/namespace.rs b/src/agent/src/namespace.rs index 20dd2d9596..8f14c8e4c7 100644 --- a/src/agent/src/namespace.rs +++ b/src/agent/src/namespace.rs @@ -178,12 +178,6 @@ impl fmt::Debug for NamespaceType { } } -impl Default for NamespaceType { - fn default() -> Self { - NamespaceType::Ipc - } -} - #[cfg(test)] mod tests { use super::{Namespace, NamespaceType};