From ba7c0ed9494dff08acfe7d5465df1840e6c574a5 Mon Sep 17 00:00:00 2001 From: Bryant Biggs Date: Tue, 19 Sep 2023 11:07:41 -0400 Subject: [PATCH 1/4] deps: Replace `regex` with the lighter weight `regex-lite` --- crates/shim/Cargo.toml | 2 +- crates/shim/src/mount.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/shim/Cargo.toml b/crates/shim/Cargo.toml index 55e55f27..444bd3c4 100644 --- a/crates/shim/Cargo.toml +++ b/crates/shim/Cargo.toml @@ -36,7 +36,7 @@ signal-hook = "0.3.13" oci-spec = "0.6.0" prctl = "1.0.0" page_size = "0.6.0" -regex = "1" +regex-lite = "0.1" containerd-shim-protos = { path = "../shim-protos", version = "0.5.0" } diff --git a/crates/shim/src/mount.rs b/crates/shim/src/mount.rs index f823a50a..af4fcd17 100644 --- a/crates/shim/src/mount.rs +++ b/crates/shim/src/mount.rs @@ -29,7 +29,7 @@ use log::error; use nix::mount::{mount, MsFlags}; #[cfg(target_os = "linux")] use nix::unistd::{fork, ForkResult}; -use regex::Regex; +use regex_lite::Regex; use crate::error::{Error, Result}; #[cfg(not(feature = "async"))] @@ -273,7 +273,7 @@ fn longest_common_prefix(dirs: &[String]) -> Option { } // NOTE: the snapshot id is based on digits. -// in order to avoid to get snapshots/x, shoule be back to parent dir. +// in order to avoid to get snapshots/x, should be back to parent dir. // however, there is assumption that the common dir is ${root}/io.containerd.v1.overlayfs/snapshots. #[cfg(target_os = "linux")] fn trim_flawed_dir(s: &str) -> String { From 9cc4f96af16904d3e84a1ba88d25822e06a549cf Mon Sep 17 00:00:00 2001 From: Bryant Biggs Date: Wed, 20 Sep 2023 09:37:25 -0400 Subject: [PATCH 2/4] refactor: Simplify longest common prefix logic --- crates/shim/src/mount.rs | 64 ++++++++++++++++------------------------ 1 file changed, 26 insertions(+), 38 deletions(-) diff --git a/crates/shim/src/mount.rs b/crates/shim/src/mount.rs index af4fcd17..cfe6a2e1 100644 --- a/crates/shim/src/mount.rs +++ b/crates/shim/src/mount.rs @@ -239,37 +239,29 @@ fn options_size(options: &[String]) -> usize { options.iter().fold(0, |sum, x| sum + x.len()) } -fn longest_common_prefix(dirs: &[String]) -> Option { +fn longest_common_prefix(dirs: &[String]) -> &str { if dirs.is_empty() { - return None; - } - if dirs.len() == 1 { - if dirs[0].is_empty() { - return None; - } - return Some(dirs[0].to_string()); + return ""; } - let min = dirs.iter().min().unwrap(); - let max = dirs.iter().max().unwrap(); - let min_chars = min.chars().collect::>(); - let max_chars = max.chars().collect::>(); - let mut i = 0; - while i < min_chars.len() && i < max_chars.len() { - if min_chars[i] != max_chars[i] { - if i == 0 { - return None; + let first_dir = &dirs[0]; + + for (i, byte) in first_dir.as_bytes().iter().enumerate() { + for dir in dirs { + if dir.as_bytes().get(i) != Some(byte) { + + let mut end = i; + // guaranteed not to underflow since is_char_boundary(0) is always true + while !first_dir.is_char_boundary(end) { + end -= 1; + } + + return &first_dir[0..end] } - return Some(min[0..i].to_string()); } - i += 1; } - if min.is_empty() { - None - } else { - Some(min.to_string()) - } + first_dir } // NOTE: the snapshot id is based on digits. @@ -282,6 +274,7 @@ fn trim_flawed_dir(s: &str) -> String { } #[cfg(target_os = "linux")] +#[derive(Default)] struct LowerdirCompactor { options: Vec, lowerdirs: Option>, @@ -291,11 +284,7 @@ struct LowerdirCompactor { #[cfg(target_os = "linux")] impl LowerdirCompactor { fn new(options: &[String]) -> Self { - Self { - options: options.to_vec(), - lowerdirs: None, - lowerdir_prefix: None, - } + Self::default() } fn lowerdirs(&mut self) -> &mut Self { @@ -317,8 +306,7 @@ impl LowerdirCompactor { .as_ref() .filter(|x| x.len() > 1) .map(|x| longest_common_prefix(x)) - .unwrap_or(None) - .filter(|x| x != "/") + .filter(|x| *x != "/") .map(|x| trim_flawed_dir(&x)) .filter(|x| !x.is_empty() && x != "/"); self @@ -632,21 +620,21 @@ mod tests { #[test] fn test_longest_common_prefix() { - let mut tcases: Vec<(Vec, Option)> = Vec::new(); - tcases.push((vec![], None)); - tcases.push((vec!["foo".to_string()], Some("foo".to_string()))); - tcases.push((vec!["foo".to_string(), "bar".to_string()], None)); + let mut tcases: Vec<(Vec, String)> = Vec::new(); + tcases.push((vec![], "".to_string())); + tcases.push((vec!["foo".to_string()], "foo".to_string())); + tcases.push((vec!["foo".to_string(), "bar".to_string()], "".to_string())); tcases.push(( vec!["foo".to_string(), "foo".to_string()], - Some("foo".to_string()), + "foo".to_string(), )); tcases.push(( vec!["foo".to_string(), "foobar".to_string()], - Some("foo".to_string()), + "foo".to_string(), )); tcases.push(( vec!["foo".to_string(), "".to_string(), "foobar".to_string()], - None, + "".to_string(), )); for (case, expected) in tcases { let res = longest_common_prefix(&case); From e15d7357c18da4171c9e3088dcbc2640fbc4e8a4 Mon Sep 17 00:00:00 2001 From: Bryant Biggs Date: Wed, 20 Sep 2023 10:12:38 -0400 Subject: [PATCH 3/4] feat: Replace use of regex crate --- crates/shim/Cargo.toml | 1 - crates/shim/src/mount.rs | 27 +++++++++++++++++++-------- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/crates/shim/Cargo.toml b/crates/shim/Cargo.toml index 444bd3c4..bcc1d1b7 100644 --- a/crates/shim/Cargo.toml +++ b/crates/shim/Cargo.toml @@ -36,7 +36,6 @@ signal-hook = "0.3.13" oci-spec = "0.6.0" prctl = "1.0.0" page_size = "0.6.0" -regex-lite = "0.1" containerd-shim-protos = { path = "../shim-protos", version = "0.5.0" } diff --git a/crates/shim/src/mount.rs b/crates/shim/src/mount.rs index cfe6a2e1..a2658960 100644 --- a/crates/shim/src/mount.rs +++ b/crates/shim/src/mount.rs @@ -29,7 +29,6 @@ use log::error; use nix::mount::{mount, MsFlags}; #[cfg(target_os = "linux")] use nix::unistd::{fork, ForkResult}; -use regex_lite::Regex; use crate::error::{Error, Result}; #[cfg(not(feature = "async"))] @@ -249,14 +248,13 @@ fn longest_common_prefix(dirs: &[String]) -> &str { for (i, byte) in first_dir.as_bytes().iter().enumerate() { for dir in dirs { if dir.as_bytes().get(i) != Some(byte) { - let mut end = i; // guaranteed not to underflow since is_char_boundary(0) is always true while !first_dir.is_char_boundary(end) { end -= 1; } - return &first_dir[0..end] + return &first_dir[0..end]; } } } @@ -269,8 +267,19 @@ fn longest_common_prefix(dirs: &[String]) -> &str { // however, there is assumption that the common dir is ${root}/io.containerd.v1.overlayfs/snapshots. #[cfg(target_os = "linux")] fn trim_flawed_dir(s: &str) -> String { - let r = Regex::new(r"((/[^/]+)+/)([^/]*)").unwrap(); - r.replace(s, "$1").to_string() + match s.ends_with('/') { + true => s.to_owned(), + false => { + // Iterate in reverse order to find the last '/', then return string in correct order + s.chars() + .rev() + .skip_while(|x| *x != '/') + .collect::>() + .iter() + .rev() + .collect::() + } + } } #[cfg(target_os = "linux")] @@ -284,7 +293,10 @@ struct LowerdirCompactor { #[cfg(target_os = "linux")] impl LowerdirCompactor { fn new(options: &[String]) -> Self { - Self::default() + Self { + options: options.to_vec(), + ..Self::default() + } } fn lowerdirs(&mut self) -> &mut Self { @@ -306,8 +318,7 @@ impl LowerdirCompactor { .as_ref() .filter(|x| x.len() > 1) .map(|x| longest_common_prefix(x)) - .filter(|x| *x != "/") - .map(|x| trim_flawed_dir(&x)) + .map(trim_flawed_dir) .filter(|x| !x.is_empty() && x != "/"); self } From 9e2c7a7113a1858b8e273fe94d1abd35a09b8ea9 Mon Sep 17 00:00:00 2001 From: Bryant Biggs Date: Wed, 20 Sep 2023 13:59:24 -0400 Subject: [PATCH 4/4] fix: Use `rfind` to slice string up to last `/` --- crates/shim/src/mount.rs | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/crates/shim/src/mount.rs b/crates/shim/src/mount.rs index a2658960..9c93c8c6 100644 --- a/crates/shim/src/mount.rs +++ b/crates/shim/src/mount.rs @@ -267,19 +267,7 @@ fn longest_common_prefix(dirs: &[String]) -> &str { // however, there is assumption that the common dir is ${root}/io.containerd.v1.overlayfs/snapshots. #[cfg(target_os = "linux")] fn trim_flawed_dir(s: &str) -> String { - match s.ends_with('/') { - true => s.to_owned(), - false => { - // Iterate in reverse order to find the last '/', then return string in correct order - s.chars() - .rev() - .skip_while(|x| *x != '/') - .collect::>() - .iter() - .rev() - .collect::() - } - } + s[0..s.rfind('/').unwrap_or(0) + 1].to_owned() } #[cfg(target_os = "linux")] @@ -619,6 +607,8 @@ mod tests { #[test] fn test_trim_flawed_dir() { let mut tcases: Vec<(&str, String)> = Vec::new(); + tcases.push(("/", "/".to_string())); + tcases.push(("/foo", "/".to_string())); tcases.push(("/.foo-_bar/foo", "/.foo-_bar/".to_string())); tcases.push(("/.foo-_bar/foo/", "/.foo-_bar/foo/".to_string())); tcases.push(("/.foo-_bar/foo/bar", "/.foo-_bar/foo/".to_string()));