From 7d49859e8439807b8490c5fdd8e44f1d4d8282b0 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 09:41:59 +0100 Subject: [PATCH 01/30] Use ? on Option where possible I looked for functions that returned Option like get_user_id() before this commit. This function calls some other function, check if it returns a value (Option::Some), and then does something with this value. If a None was seen, a None is explicitly returned. This can be simplified with the question mark operator. By using ? on the Option, the function automatically returns None if a None is seen. I think this is a bit more readable (although perhaps less beginner friendly since this might read like magic). Some functions did similar things but with a Result. Here, I used .ok()? to get the desired behaviour. I still feel like this is simpler. Signed-off-by: Uli Schlachter --- src/main.rs | 75 ++++++++++++++++++++++++---------------------------- src/utils.rs | 5 +--- 2 files changed, 35 insertions(+), 45 deletions(-) diff --git a/src/main.rs b/src/main.rs index 1bb8cc5..60b6a80 100644 --- a/src/main.rs +++ b/src/main.rs @@ -522,14 +522,14 @@ fn run_udevd() -> Option> { } fn get_guest_tools_dir() -> Option { - if let Ok(current_exe) = env::current_exe() { - if let Some(parent_dir) = current_exe.parent()?.parent() { - if let Some(dir) = parent_dir.to_str() { - return Some(dir.to_string()); - } - } - } - None + Some( + env::current_exe() + .ok()? + .parent()? + .parent()? + .to_str()? + .to_string(), + ) } fn _get_network_device_from_entries(entries: std::fs::ReadDir) -> Option { @@ -569,26 +569,25 @@ fn get_network_device() -> Option { fn setup_network() -> Option> { utils::run_cmd("ip", &["link", "set", "dev", "lo", "up"]); - if let Ok(cmdline) = std::fs::read_to_string("/proc/cmdline") { - if cmdline.contains("virtme.dhcp") { - if let Some(guest_tools_dir) = get_guest_tools_dir() { - if let Some(network_dev) = get_network_device() { - utils::log(&format!("setting up network device {}", network_dev)); - let handle = thread::spawn(move || { - let args = [ - "udhcpc", - "-i", - &network_dev, - "-n", - "-q", - "-f", - "-s", - &format!("{}/virtme-udhcpc-script", guest_tools_dir), - ]; - utils::run_cmd("busybox", &args); - }); - return Some(handle); - } + let cmdline = std::fs::read_to_string("/proc/cmdline").ok()?; + if cmdline.contains("virtme.dhcp") { + if let Some(guest_tools_dir) = get_guest_tools_dir() { + if let Some(network_dev) = get_network_device() { + utils::log(&format!("setting up network device {}", network_dev)); + let handle = thread::spawn(move || { + let args = [ + "udhcpc", + "-i", + &network_dev, + "-n", + "-q", + "-f", + "-s", + &format!("{}/virtme-udhcpc-script", guest_tools_dir), + ]; + utils::run_cmd("busybox", &args); + }); + return Some(handle); } } } @@ -599,19 +598,13 @@ fn extract_user_script(virtme_script: &str) -> Option { let start_marker = "virtme.exec=`"; let end_marker = "`"; - if let Some(start_index) = virtme_script.find(start_marker) { - let start_index = start_index + start_marker.len(); - if let Some(end_index) = virtme_script[start_index..].find(end_marker) { - let encoded_cmd = &virtme_script[start_index..start_index + end_index]; - if let Ok(decoded_bytes) = BASE64.decode(encoded_cmd) { - if let Ok(decoded_string) = String::from_utf8(decoded_bytes) { - return Some(decoded_string); - } - } - } - } - - None + let start_index = virtme_script.find(start_marker)?; + let start_index = start_index + start_marker.len(); + let end_index = virtme_script[start_index..].find(end_marker)?; + let encoded_cmd = &virtme_script[start_index..start_index + end_index]; + let decoded_bytes = BASE64.decode(encoded_cmd).ok()?; + let decoded_string = String::from_utf8(decoded_bytes).ok()?; + Some(decoded_string) } fn run_user_script() { diff --git a/src/utils.rs b/src/utils.rs index 6213a34..c3b62cd 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -36,10 +36,7 @@ pub fn log(msg: &str) { } pub fn get_user_id(username: &str) -> Option { - if let Some(user) = get_user_by_name(username) { - return Some(user.uid()); - } - None + Some(get_user_by_name(username)?.uid()) } pub fn do_chown(path: &str, uid: u32, gid: u32) -> io::Result<()> { From 9c429bc4e78a3659e544d3da80e99f6387b82c90 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 09:48:12 +0100 Subject: [PATCH 02/30] Add a simple test for extract_user_script() I want to change this function next. To make sure I am not breaking anything, I am adding a simple test for it. Signed-off-by: Uli Schlachter --- src/main.rs | 3 +++ src/test.rs | 10 ++++++++++ 2 files changed, 13 insertions(+) create mode 100644 src/test.rs diff --git a/src/main.rs b/src/main.rs index 60b6a80..ad6560c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -33,6 +33,9 @@ use std::thread; use std::time::Duration; mod utils; +#[cfg(test)] +mod test; + struct MountInfo { source: &'static str, target: &'static str, diff --git a/src/test.rs b/src/test.rs new file mode 100644 index 0000000..24c8cc3 --- /dev/null +++ b/src/test.rs @@ -0,0 +1,10 @@ +// SPDX-License-Identifier: GPL-3.0 + +#[test] +fn test_extract_user_script() { + let input = "other=stuff virtme.exec=`SGVsbG8K` is=ignored"; + assert_eq!( + super::extract_user_script(input), + Some("Hello\n".to_string()) + ); +} From 99b099277f54889f5efe42d98cd5838702241e97 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 09:51:11 +0100 Subject: [PATCH 03/30] Simplify extract_user_script() This function used find() and manual indexing to split up an input string. This is all fine, but not terribly obvious to read. In this commit, I change the code to use split_once() instead. This finds the first occurrence of some search string and returns the substrings before and after this. I think this makes the function much more obvious. Additionally, I changed end_marker from a string to a char. Both split_once() and the previously used find() can get a string or a character as argument. I guess a single character can be slightly more efficient, but I do not actually know. It just seemed to be the right thing. Signed-off-by: Uli Schlachter --- src/main.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/main.rs b/src/main.rs index ad6560c..b0424da 100644 --- a/src/main.rs +++ b/src/main.rs @@ -599,15 +599,11 @@ fn setup_network() -> Option> { fn extract_user_script(virtme_script: &str) -> Option { let start_marker = "virtme.exec=`"; - let end_marker = "`"; - - let start_index = virtme_script.find(start_marker)?; - let start_index = start_index + start_marker.len(); - let end_index = virtme_script[start_index..].find(end_marker)?; - let encoded_cmd = &virtme_script[start_index..start_index + end_index]; - let decoded_bytes = BASE64.decode(encoded_cmd).ok()?; - let decoded_string = String::from_utf8(decoded_bytes).ok()?; - Some(decoded_string) + let end_marker = '`'; + + let (_before, remaining) = virtme_script.split_once(start_marker)?; + let (encoded_cmd, _after) = remaining.split_once(end_marker)?; + Some(String::from_utf8(BASE64.decode(encoded_cmd).ok()?).ok()?) } fn run_user_script() { From a1cf62f6ae8e7cf054fe3ffe8e807d025e7cc037 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 09:54:33 +0100 Subject: [PATCH 04/30] Remove use of HashMap The code here created a Vec, then turned that into a HashMap, then iterated over the HashMap. Instead, this could just iterate over the Vec directly. In fact, a plain array would suffice and the extra allocation for the Vec is not needed. Signed-off-by: Uli Schlachter --- src/main.rs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/main.rs b/src/main.rs index b0424da..c55161c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -20,7 +20,6 @@ use nix::libc; use nix::sys::reboot; use nix::sys::stat::Mode; use nix::unistd::sethostname; -use std::collections::HashMap; use std::env; use std::ffi::CStr; use std::fs::{File, OpenOptions}; @@ -343,14 +342,12 @@ fn set_cwd() { } fn symlink_fds() { - let fd_links: HashMap<&str, &str> = vec![ + let fd_links = [ ("/proc/self/fd", "/dev/fd"), ("/proc/self/fd/0", "/dev/stdin"), ("/proc/self/fd/1", "/dev/stdout"), ("/proc/self/fd/2", "/dev/stderr"), - ] - .into_iter() - .collect(); + ]; // Install /proc/self/fd symlinks into /dev if not already present. for (src, dst) in fd_links.iter() { @@ -618,13 +615,11 @@ fn run_user_script() { ); } else { // Re-create stdout/stderr to connect to the virtio-serial ports. - let io_files: HashMap<&str, &str> = vec![ + let io_files = [ ("/dev/virtio-ports/virtme.dev_stdin", "/dev/stdin"), ("/dev/virtio-ports/virtme.dev_stdout", "/dev/stdout"), ("/dev/virtio-ports/virtme.dev_stderr", "/dev/stderr"), - ] - .into_iter() - .collect(); + ]; for (src, dst) in io_files.iter() { if std::path::Path::new(dst).exists() { utils::do_unlink(dst); From 44b9d9591bb32e0cf893ed6ce49a6d7f371d7b25 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 09:56:20 +0100 Subject: [PATCH 05/30] Remove Option return from run_misc_services() THis function cannot fail and the Option is basically dead code. Additionally, I removed the "move" on this lambda since the lambda does not capture anything, so nothing is actually moved. Signed-off-by: Uli Schlachter --- src/main.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/main.rs b/src/main.rs index c55161c..54d2b92 100644 --- a/src/main.rs +++ b/src/main.rs @@ -885,15 +885,14 @@ fn run_snapd() { } } -fn run_misc_services() -> Option> { - let handle = thread::spawn(move || { +fn run_misc_services() -> thread::JoinHandle<()> { + thread::spawn(|| { symlink_fds(); mount_virtme_initmounts(); fix_packaging_files(); override_system_files(); run_snapd(); - }); - Some(handle) + }) } fn print_logo() { @@ -925,7 +924,7 @@ fn main() { let mut handles: Vec>> = Vec::new(); handles.push(run_udevd()); handles.push(setup_network()); - handles.push(run_misc_services()); + handles.push(Some(run_misc_services())); // Wait for the completion of the detached services. for handle in handles.into_iter().flatten() { From ffa33fba9998c77fc2a19bf0848c1a9fe3a15b6c Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 10:00:19 +0100 Subject: [PATCH 06/30] Simplify an env var check Instead of checking if the env variable is set and then early-returning, this commit reverses the order so that the function is only run if the variable is not present. Also, instead of pattern matching with "if let", this uses is_err() to check if the variable is not set. Signed-off-by: Uli Schlachter --- src/main.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/main.rs b/src/main.rs index 54d2b92..e197f3e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -696,10 +696,9 @@ fn setup_user_script() { if let Ok(cmdline) = std::fs::read_to_string("/proc/cmdline") { if let Some(cmd) = extract_user_script(&cmdline) { create_user_script(&cmd); - if let Ok(_) = env::var("virtme_graphics") { - return; + if env::var("virtme_graphics").is_err() { + run_user_script(); } - run_user_script(); } } } From 125349dd8802f222b48c9b9631da43fb2d0bc725 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 10:06:58 +0100 Subject: [PATCH 07/30] Simplify(?) some environment lookup The code previously initiated the "uid" variable to a default value and then had two nested "if"s that both must succeed for another value to be set. In this commit, I change that to a processing chain on Option: If env::var() produces a value, Option::and_then() is used to call another function. If either of these two steps produces a None, the result will be a None. .unwrap_or() is then used to replace the None with a default value. This allows to get rid of the "mut" on the variable. Signed-off-by: Uli Schlachter --- src/main.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/main.rs b/src/main.rs index e197f3e..1cd6fb4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -760,12 +760,10 @@ fn run_shell(tty_fd: libc::c_int, args: Vec) { fn init_xdg_runtime_dir() { // Initialize XDG_RUNTIME_DIR (required to provide a better compatibility with graphic apps). - let mut uid = 0; - if let Ok(user) = env::var("virtme_user") { - if let Some(virtme_uid) = utils::get_user_id(&user) { - uid = virtme_uid; - } - } + let uid = env::var("virtme_user") + .ok() + .and_then(|user| utils::get_user_id(&user)) + .unwrap_or(0); let dir = format!("/run/user/{}", uid); utils::do_mkdir(&dir); utils::do_chown(&dir, uid, uid).ok(); From 12759fea0fbb74055a0436d07c28972cef31f667 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 10:10:28 +0100 Subject: [PATCH 08/30] Simplify lookup of $virtme_user This uses unwrap_or_else() to construct an empty string in case the virtme_user env var is not used. Signed-off-by: Uli Schlachter --- src/main.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/main.rs b/src/main.rs index 1cd6fb4..fb20d2e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -652,12 +652,7 @@ fn run_user_script() { // Determine if we need to switch to a different user, or if we can run the script as root. let cmd: String; let args: Vec<&str>; - let user: String; - if let Ok(virtme_user) = env::var("virtme_user") { - user = virtme_user; - } else { - user = String::new(); - } + let user = env::var("virtme_user").unwrap_or_else(|_| String::new()); if !user.is_empty() { cmd = "su".to_string(); args = vec![&user, "-c", USER_SCRIPT]; From f1f16aa7ac9b155fcc18fea537283f400f03224c Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 10:12:34 +0100 Subject: [PATCH 09/30] Simplify(?) some command construction "if"s are expressions, too, and can be used to set variables. This commit refactors some code to use this to set the "cmd" and "args" variables based on a condition. Additionally, "cmd" is changed from a String to a &str since both cases just use a static string. Signed-off-by: Uli Schlachter --- src/main.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/main.rs b/src/main.rs index fb20d2e..5c1ce08 100644 --- a/src/main.rs +++ b/src/main.rs @@ -650,19 +650,15 @@ fn run_user_script() { .expect("failed to open console."); // Determine if we need to switch to a different user, or if we can run the script as root. - let cmd: String; - let args: Vec<&str>; let user = env::var("virtme_user").unwrap_or_else(|_| String::new()); - if !user.is_empty() { - cmd = "su".to_string(); - args = vec![&user, "-c", USER_SCRIPT]; + let (cmd, args) = if !user.is_empty() { + ("su", vec![&user, "-c", USER_SCRIPT]) } else { - cmd = "/bin/sh".to_string(); - args = vec![USER_SCRIPT]; - } + ("/bin/sh", vec![USER_SCRIPT]) + }; clear_virtme_envs(); unsafe { - Command::new(&cmd) + Command::new(cmd) .args(&args) .pre_exec(move || { nix::libc::setsid(); From 1d1dc13befe2c8d6de9f8e142d57f5a6998a32d5 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 10:14:14 +0100 Subject: [PATCH 10/30] Remove useless Some-wrapping Clippy reports that there is code here that takes an Option and produces the exact same Option from it. Fix that. warning: question mark operator is useless here --> src/main.rs:603:5 | 603 | Some(String::from_utf8(BASE64.decode(encoded_cmd).ok()?).ok()?) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try removing question mark and `Some()`: `String::from_utf8(BASE64.decode(encoded_cmd).ok()?).ok()` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_question_mark = note: `#[warn(clippy::needless_question_mark)]` on by default Signed-off-by: Uli Schlachter --- src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.rs b/src/main.rs index 5c1ce08..2d78621 100644 --- a/src/main.rs +++ b/src/main.rs @@ -600,7 +600,7 @@ fn extract_user_script(virtme_script: &str) -> Option { let (_before, remaining) = virtme_script.split_once(start_marker)?; let (encoded_cmd, _after) = remaining.split_once(end_marker)?; - Some(String::from_utf8(BASE64.decode(encoded_cmd).ok()?).ok()?) + String::from_utf8(BASE64.decode(encoded_cmd).ok()?).ok() } fn run_user_script() { From 8a3c38123e71773e665cf660410ce615e883cbb0 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 10:15:44 +0100 Subject: [PATCH 11/30] Remove unnecessary 'static Clippy reports: warning: statics have by default a `'static` lifetime --> src/utils.rs:19:20 | 19 | static PROG_NAME: &'static str = "virtme-ng-init"; | -^^^^^^^---- help: consider removing `'static`: `&str` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_static_lifetimes = note: `#[warn(clippy::redundant_static_lifetimes)]` on by default Signed-off-by: Uli Schlachter --- src/utils.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils.rs b/src/utils.rs index c3b62cd..84643c2 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -16,7 +16,7 @@ use std::os::unix::fs::PermissionsExt; use std::process::{Command, Stdio}; use users::get_user_by_name; -static PROG_NAME: &'static str = "virtme-ng-init"; +static PROG_NAME: &str = "virtme-ng-init"; pub fn log(msg: &str) { if msg.is_empty() { From 1a6705944034c6bdf0c2f8892fb1f7c26e1031ad Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 10:20:20 +0100 Subject: [PATCH 12/30] Replace e.g. libc::S_IRWXU with Mode::S_IRWXU This code uses numeric constants from libc and then converts the resulting number into a Mode. By instead directly starting with the Mode constants, we directly get a Mode. Signed-off-by: Uli Schlachter --- src/utils.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/utils.rs b/src/utils.rs index 84643c2..3c85b02 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -4,7 +4,6 @@ //! //! Author: Andrea Righi -use nix::libc; use nix::mount::{mount, MsFlags}; use nix::sys::stat::Mode; use nix::unistd::{chown, Gid, Uid}; @@ -47,8 +46,8 @@ pub fn do_chown(path: &str, uid: u32, gid: u32) -> io::Result<()> { } pub fn do_mkdir(path: &str) { - let dmask = libc::S_IRWXU | libc::S_IRGRP | libc::S_IXGRP | libc::S_IROTH | libc::S_IXOTH; - nix::unistd::mkdir(path, Mode::from_bits_truncate(dmask as u32)).ok(); + let dmask = Mode::S_IRWXU | Mode::S_IRGRP | Mode::S_IXGRP | Mode::S_IROTH | Mode::S_IXOTH; + nix::unistd::mkdir(path, dmask).ok(); } pub fn do_unlink(path: &str) { From ea2c53451ecb3f985bb67b8ab5b67677969d7819 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 10:23:17 +0100 Subject: [PATCH 13/30] Fix clippy warnings around logging warning: `to_string` applied to a type that implements `Display` in `format!` args --> src/utils.rs:114:16 | 114 | err.to_string() | ^^^^^^^^^^^^ help: remove this | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_format_args = note: `#[warn(clippy::to_string_in_format_args)]` on by default warning: useless use of `format!` --> src/main.rs:203:21 | 203 | utils::log(&format!("must be run as PID 1")); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `"must be run as PID 1".to_string()` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_format = note: `#[warn(clippy::useless_format)]` on by default warning: useless use of `format!` --> src/main.rs:272:21 | 272 | utils::log(&format!("virtme_hostname is not defined")); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `"virtme_hostname is not defined".to_string()` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_format I did not actually replace format!() with to_string() since utils::log() just needs a &str, so we can just use a string constant (I guess clippy would propose this if I implemented its first suggestion). Signed-off-by: Uli Schlachter --- src/main.rs | 4 ++-- src/utils.rs | 7 +------ 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/main.rs b/src/main.rs index 2d78621..e6510ad 100644 --- a/src/main.rs +++ b/src/main.rs @@ -200,7 +200,7 @@ const USER_SCRIPT: &str = "/tmp/.virtme-script"; fn check_init_pid() { if id() != 1 { - utils::log(&format!("must be run as PID 1")); + utils::log("must be run as PID 1"); exit(1); } } @@ -269,7 +269,7 @@ fn configure_hostname() { utils::log(&format!("failed to change hostname: {}", err)); } } else { - utils::log(&format!("virtme_hostname is not defined")); + utils::log("virtme_hostname is not defined"); } } diff --git a/src/utils.rs b/src/utils.rs index 3c85b02..a9aa976 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -107,12 +107,7 @@ pub fn do_mount(source: &str, target: &str, fstype: &str, flags: usize, fsdata: Some(fsdata_cstr.as_ref()), ); if let Err(err) = result { - log(&format!( - "mount {} -> {}: {}", - source, - target, - err.to_string() - )); + log(&format!("mount {} -> {}: {}", source, target, err)); } } From a74c0e648e3c2abd05ab022402bbb5384d42a74a Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 10:27:01 +0100 Subject: [PATCH 14/30] Fix clippy warnings about patterns warning: single-character string constant used as pattern --> src/main.rs:395:30 | 395 | &key.replace("_", "."), | ^^^ help: try using a `char` instead: `'_'` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern = note: `#[warn(clippy::single_char_pattern)]` on by default warning: single-character string constant used as pattern --> src/main.rs:894:44 | 894 | println!("{}", logo.trim_start_matches("\n")); | ^^^^ help: try using a `char` instead: `'\n'` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern Signed-off-by: Uli Schlachter --- src/main.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main.rs b/src/main.rs index e6510ad..4efc0af 100644 --- a/src/main.rs +++ b/src/main.rs @@ -392,7 +392,7 @@ fn mount_virtme_initmounts() { if key.starts_with("virtme_initmount") { utils::do_mkdir(&path); utils::do_mount( - &key.replace("_", "."), + &key.replace('_', "."), &path, "9p", 0, @@ -891,7 +891,7 @@ fn print_logo() { \ V /| | | | |_| | | | | | __/_____| | | | (_| | \_/ |_|_| \__|_| |_| |_|\___| |_| |_|\__ | |___/"#; - println!("{}", logo.trim_start_matches("\n")); + println!("{}", logo.trim_start_matches('\n')); println!(" kernel version: {}\n", get_kernel_version(true)); } From d6e495b730253ffd8f4ca5d40b4660f181d5b3d2 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 10:28:02 +0100 Subject: [PATCH 15/30] Fix clippy warning about unnecessary return warning: unneeded `return` statement --> src/main.rs:552:5 | 552 | return None; | ^^^^^^^^^^^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return = note: `#[warn(clippy::needless_return)]` on by default = help: remove `return` Signed-off-by: Uli Schlachter --- src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.rs b/src/main.rs index 4efc0af..fa2905e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -549,7 +549,7 @@ fn _get_network_device_from_entries(entries: std::fs::ReadDir) -> Option } } } - return None; + None } fn get_network_device() -> Option { From 8e4452fca1c5825e37f3f97bcc9a3cb8704dee64 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 10:30:54 +0100 Subject: [PATCH 16/30] Fix clippy warnings about reading lines warning: unnecessary `if let` since only the `Ok` variant of the iterator element is used --> src/main.rs:249:13 | 249 | for line in reader.lines() { | ^ -------------- help: try: `reader.lines().flatten()` | _____________| | | 250 | | if let Ok(line) = line { 251 | | if line.chars().nth(27) == Some('C') { 252 | | let console = line.split(' ').next()?.to_string(); ... | 255 | | } 256 | | } | |_____________^ | help: ...and remove the `if let` statement in the for loop --> src/main.rs:250:17 | 250 | / if let Ok(line) = line { 251 | | if line.chars().nth(27) == Some('C') { 252 | | let console = line.split(' ').next()?.to_string(); 253 | | return Some(format!("/dev/{}", console)); 254 | | } 255 | | } | |_________________^ = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_flatten = note: `#[warn(clippy::manual_flatten)]` on by default warning: unnecessary `if let` since only the `Ok` variant of the iterator element is used --> src/main.rs:536:5 | 536 | for entry in entries { | ^ ------- help: try: `entries.flatten()` | _____| | | 537 | | if let Ok(entry) = entry { 538 | | let path = entry.path(); 539 | | if !path.is_dir() { ... | 550 | | } 551 | | } | |_____^ | help: ...and remove the `if let` statement in the for loop --> src/main.rs:537:9 | 537 | / if let Ok(entry) = entry { 538 | | let path = entry.path(); 539 | | if !path.is_dir() { 540 | | continue; ... | 549 | | } 550 | | } | |_________^ = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_flatten warning: unnecessary `if let` since only the `Ok` variant of the iterator element is used --> src/main.rs:543:17 | 543 | for entry in net_entries { | ^ ----------- help: try: `net_entries.flatten()` | _________________| | | 544 | | if let Ok(entry) = entry { 545 | | let path = entry.path().file_name()?.to_string_lossy().to_string(); 546 | | return Some(path); 547 | | } 548 | | } | |_________________^ | help: ...and remove the `if let` statement in the for loop --> src/main.rs:544:21 | 544 | / if let Ok(entry) = entry { 545 | | let path = entry.path().file_name()?.to_string_lossy().to_string(); 546 | | return Some(path); 547 | | } | |_____________________^ = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_flatten Signed-off-by: Uli Schlachter --- src/main.rs | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/src/main.rs b/src/main.rs index fa2905e..62d8a90 100644 --- a/src/main.rs +++ b/src/main.rs @@ -246,12 +246,11 @@ fn get_active_console() -> Option { Ok(file) => { let reader = BufReader::new(file); - for line in reader.lines() { - if let Ok(line) = line { - if line.chars().nth(27) == Some('C') { - let console = line.split(' ').next()?.to_string(); - return Some(format!("/dev/{}", console)); - } + // .flatten() ignores lines with reading errors + for line in reader.lines().flatten() { + if line.chars().nth(27) == Some('C') { + let console = line.split(' ').next()?.to_string(); + return Some(format!("/dev/{}", console)); } } None @@ -533,19 +532,17 @@ fn get_guest_tools_dir() -> Option { } fn _get_network_device_from_entries(entries: std::fs::ReadDir) -> Option { - for entry in entries { - if let Ok(entry) = entry { - let path = entry.path(); - if !path.is_dir() { - continue; - } - if let Ok(net_entries) = std::fs::read_dir(path.join("net")) { - for entry in net_entries { - if let Ok(entry) = entry { - let path = entry.path().file_name()?.to_string_lossy().to_string(); - return Some(path); - } - } + // .flatten() ignores lines with reading errors + for entry in entries.flatten() { + let path = entry.path(); + if !path.is_dir() { + continue; + } + if let Ok(net_entries) = std::fs::read_dir(path.join("net")) { + // .flatten() ignores lines with reading errors + for entry in net_entries.flatten() { + let path = entry.path().file_name()?.to_string_lossy().to_string(); + return Some(path); } } } From 140f98bf7a3a6882701c5044f8df48565f9bb4ee Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 10:32:34 +0100 Subject: [PATCH 17/30] Fix clippy warning about "loop never loops" This warning was introduced with the previous commit. error: this loop never actually loops --> src/main.rs:543:13 | 543 | / for entry in net_entries.flatten() { 544 | | let path = entry.path().file_name()?.to_string_lossy().to_string(); 545 | | return Some(path); 546 | | } | |_____________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#never_loop = note: `#[deny(clippy::never_loop)]` on by default help: if you need the first element of the iterator, try writing | 543 | if let Some(entry) = net_entries.flatten().next() { | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Signed-off-by: Uli Schlachter --- src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.rs b/src/main.rs index 62d8a90..3e1754a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -540,7 +540,7 @@ fn _get_network_device_from_entries(entries: std::fs::ReadDir) -> Option } if let Ok(net_entries) = std::fs::read_dir(path.join("net")) { // .flatten() ignores lines with reading errors - for entry in net_entries.flatten() { + if let Some(entry) = net_entries.flatten().next() { let path = entry.path().file_name()?.to_string_lossy().to_string(); return Some(path); } From ae7980f14b56e01994c915bf39c6c8887419a15f Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 10:33:55 +0100 Subject: [PATCH 18/30] Fix clippy warning about "push() after creation" I also got rid of the type annotation since it doesn't seem to be necessary. warning: calls to `push` immediately after creation --> src/main.rs:909:5 | 909 | / let mut handles: Vec>> = Vec::new(); 910 | | handles.push(run_udevd()); 911 | | handles.push(setup_network()); 912 | | handles.push(Some(run_misc_services())); | |____________________________________________^ help: consider using the `vec![]` macro: `let handles: Vec>> = vec![..];` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#vec_init_then_push = note: `#[warn(clippy::vec_init_then_push)]` on by default Signed-off-by: Uli Schlachter --- src/main.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/main.rs b/src/main.rs index 3e1754a..5708cf0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -906,10 +906,7 @@ fn main() { run_systemd_tmpfiles(); // Service initialization (some services can be parallelized here). - let mut handles: Vec>> = Vec::new(); - handles.push(run_udevd()); - handles.push(setup_network()); - handles.push(Some(run_misc_services())); + let handles = vec![run_udevd(), setup_network(), Some(run_misc_services())]; // Wait for the completion of the detached services. for handle in handles.into_iter().flatten() { From e4fedd9892ebbe324734826880ac4897276691cc Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 10:35:22 +0100 Subject: [PATCH 19/30] Fix clippy warning about unnecessary matching warning: redundant pattern matching, consider using `is_ok()` --> src/main.rs:825:12 | 825 | if let Ok(_) = env::var("virtme_graphics") { | -------^^^^^------------------------------ help: try this: `if env::var("virtme_graphics").is_ok()` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pattern_matching = note: `#[warn(clippy::redundant_pattern_matching)]` on by default Signed-off-by: Uli Schlachter --- src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.rs b/src/main.rs index 5708cf0..c7dbc16 100644 --- a/src/main.rs +++ b/src/main.rs @@ -822,7 +822,7 @@ fn run_user_session() { let tty_fd = open(consdev.as_str(), OFlag::from_bits_truncate(flags), mode) .expect("failed to open console"); - if let Ok(_) = env::var("virtme_graphics") { + if env::var("virtme_graphics").is_ok() { run_user_gui(tty_fd); } else { run_user_shell(tty_fd); From 4447d7edeac447c68aef5dd088fe424621d9d926 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 10:43:52 +0100 Subject: [PATCH 20/30] Simplify opening of TTYs Instead of repeating the same code three times, this adds a helper lambda function that does the repetitive part and calls it three times. Instead of using flags from libc and transferring them to nix, this directly uses nix's OFlag. Signed-off-by: Uli Schlachter --- src/main.rs | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/src/main.rs b/src/main.rs index c7dbc16..0623788 100644 --- a/src/main.rs +++ b/src/main.rs @@ -625,26 +625,11 @@ fn run_user_script() { } // Detach the process from the controlling terminal - let flags = libc::O_RDWR; - let mode = Mode::empty(); - let tty_in = open( - "/dev/virtio-ports/virtme.stdin", - OFlag::from_bits_truncate(flags), - mode, - ) - .expect("failed to open console."); - let tty_out = open( - "/dev/virtio-ports/virtme.stdout", - OFlag::from_bits_truncate(flags), - mode, - ) - .expect("failed to open console."); - let tty_err = open( - "/dev/virtio-ports/virtme.stderr", - OFlag::from_bits_truncate(flags), - mode, - ) - .expect("failed to open console."); + let open_tty = + |path| open(path, OFlag::O_RDWR, Mode::empty()).expect("failed to open console."); + let tty_in = open_tty("/dev/virtio-ports/virtme.stdin"); + let tty_out = open_tty("/dev/virtio-ports/virtme.stdout"); + let tty_err = open_tty("/dev/virtio-ports/virtme.stderr"); // Determine if we need to switch to a different user, or if we can run the script as root. let user = env::var("virtme_user").unwrap_or_else(|_| String::new()); From 0cc1ad30c44bb82b6dcbcd4e65fbbc871c1351e5 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 10:51:54 +0100 Subject: [PATCH 21/30] Use uname() from nix instead of libc This allows to get rid of all the unsafety and the conversion to CStr (well, OsStr now) is handled by nix for us. I also delayed the call to into_owned() a bit. This is used to convert a Cow<'_, str> to String, but the "show_machine" case does not actually need it. Thus, this removes a memory allocation. Signed-off-by: Uli Schlachter --- src/main.rs | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/src/main.rs b/src/main.rs index 0623788..17ee03f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -14,17 +14,15 @@ use base64::engine::general_purpose::STANDARD as BASE64; use base64::engine::Engine as _; -use libc::{uname, utsname}; use nix::fcntl::{open, OFlag}; use nix::libc; use nix::sys::reboot; use nix::sys::stat::Mode; +use nix::sys::utsname::uname; use nix::unistd::sethostname; use std::env; -use std::ffi::CStr; use std::fs::{File, OpenOptions}; use std::io::{self, BufRead, BufReader, BufWriter, Write}; -use std::mem; use std::os::unix::process::CommandExt; use std::path::{Path, PathBuf}; use std::process::{exit, id, Command, Stdio}; @@ -221,22 +219,16 @@ fn configure_environment() { } fn get_kernel_version(show_machine: bool) -> String { - unsafe { - let mut utsname: utsname = mem::zeroed(); - if uname(&mut utsname) == -1 { - return String::from("None"); - } - let release = CStr::from_ptr(utsname.release.as_ptr()) - .to_string_lossy() - .into_owned(); - if show_machine { - let machine = CStr::from_ptr(utsname.machine.as_ptr()) - .to_string_lossy() - .into_owned(); - format!("{} {}", release, machine) - } else { - release - } + let utsname = match uname() { + Ok(utsname) => utsname, + Err(_) => return "None".to_string(), + }; + let release = utsname.release().to_string_lossy(); + if show_machine { + let machine = utsname.machine().to_string_lossy(); + format!("{} {}", release, machine) + } else { + release.into_owned() } } From f072e049b67edb284b5e08f7ffa66cb9c7be5f30 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 10:56:35 +0100 Subject: [PATCH 22/30] Remove useless to_string() The value is passed to format!() and this does not need a separate allocation, but is totally fine with getting a &str. Signed-off-by: Uli Schlachter --- src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.rs b/src/main.rs index 17ee03f..a12786b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -241,7 +241,7 @@ fn get_active_console() -> Option { // .flatten() ignores lines with reading errors for line in reader.lines().flatten() { if line.chars().nth(27) == Some('C') { - let console = line.split(' ').next()?.to_string(); + let console = line.split(' ').next()?; return Some(format!("/dev/{}", console)); } } From 60740e731eae05a99dd4ae2c7f53c0a1f2cdf4da Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 10:58:13 +0100 Subject: [PATCH 23/30] Simplify /etc/shadow generation This function just needs to part until the first ':', so there is no need to split out all the fields and collect this into a Vec. Just use split_once() to get the first part. Signed-off-by: Uli Schlachter --- src/main.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/main.rs b/src/main.rs index a12786b..b45f921 100644 --- a/src/main.rs +++ b/src/main.rs @@ -290,11 +290,7 @@ fn generate_shadow() -> io::Result<()> { let mut writer = BufWriter::new(output_file); for line in reader.lines() { - let line = line?; - let parts: Vec<&str> = line.split(':').collect(); - - if !parts.is_empty() { - let username = parts[0]; + if let Some((username, _)) = line?.split_once(':') { writeln!(writer, "{}:!:::::::", username)?; } } From 55dac47e4fafadfe65d7c25bda74e3f94860f9dc Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 11:00:11 +0100 Subject: [PATCH 24/30] Use utils::create_file() to write some file contents I don't see why this code uses create_file() with empty contents and then writes the expected contents itself. This commit just changes it to let create_file() write the expected contents. Signed-off-by: Uli Schlachter --- src/main.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/main.rs b/src/main.rs index b45f921..418d26e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -302,13 +302,11 @@ fn generate_shadow() -> io::Result<()> { fn generate_sudoers() -> io::Result<()> { if let Ok(user) = env::var("virtme_user") { let fname = "/tmp/sudoers"; - utils::create_file(fname, 0o0440, "").ok(); - let mut file = File::create(fname)?; let content = format!( "root ALL = (ALL) NOPASSWD: ALL\n{} ALL = (ALL) NOPASSWD: ALL\n", user ); - file.write_all(content.as_bytes())?; + utils::create_file(fname, 0o0440, &content).ok(); utils::do_mount(fname, "/etc/sudoers", "", libc::MS_BIND as usize, ""); } Ok(()) From 992e5f868ba56393fbe12930ecdcb62e8d9dad81 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 11:08:33 +0100 Subject: [PATCH 25/30] Remove an unnecessary Vec An array is just as good as a Vec here and avoids a memory allocation. Signed-off-by: Uli Schlachter --- src/main.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main.rs b/src/main.rs index 418d26e..7865709 100644 --- a/src/main.rs +++ b/src/main.rs @@ -422,12 +422,12 @@ fn fix_dpkg_locks() { if !Path::new("/var/lib/dpkg").exists() { return; } - let lock_files = vec![ + let lock_files = [ "/var/lib/dpkg/lock", "/var/lib/dpkg/lock-frontend", "/var/lib/dpkg/triggers/Lock", ]; - for path in &lock_files { + for path in lock_files { let fname = Path::new(path) .file_name() .and_then(|name| name.to_str()) From 457bc572954c5a9beeddbb1faf64b3ed4649af63 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 11:23:22 +0100 Subject: [PATCH 26/30] Refactor logic in find_udevd() The code checks a series of paths for existence. The first one that is found determines the function return value. First, two static candidates are checked. Then, for each entry in $PATH, it is checked whether "udevd" exists in that directory. This commit refactors that. The if-chain is turned into iterators. First, the static candidates are put into an array. Then, $PATH is checked and each entry is mapped to a PathBuf with "udevd" appended. These two iterators are then chained and the first entry that exists is returned. This also automatically produces None if no entry exists. This introduces new allocations for the iterator producing PathBufs. However, since iterators are lazy, these allocations only actually happen when the element is needed. Thus, this should be mostly equivalent to the previous code, except that $PATH is always looked up. Signed-off-by: Uli Schlachter --- src/main.rs | 31 +++++++++++-------------------- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/src/main.rs b/src/main.rs index 7865709..7e7ec8b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -464,26 +464,17 @@ fn disable_uevent_helper() { } fn find_udevd() -> Option { - let mut udevd = PathBuf::new(); - - if PathBuf::from("/usr/lib/systemd/systemd-udevd").exists() { - udevd = PathBuf::from("/usr/lib/systemd/systemd-udevd"); - } else if PathBuf::from("/lib/systemd/systemd-udevd").exists() { - udevd = PathBuf::from("/lib/systemd/systemd-udevd"); - } else if let Ok(path) = env::var("PATH") { - for dir in path.split(':') { - let udevd_path = PathBuf::from(dir).join("udevd"); - if udevd_path.exists() { - udevd = udevd_path; - break; - } - } - } - if udevd.exists() { - Some(udevd) - } else { - None - } + let static_candidates = [ + PathBuf::from("/usr/lib/systemd/systemd-udevd"), + PathBuf::from("/lib/systemd/systemd-udevd"), + ]; + let path = env::var("PATH").unwrap_or_else(|_| String::new()); + let path_candidates = path.split(':').map(|dir| PathBuf::from(dir).join("udevd")); + + static_candidates + .into_iter() + .chain(path_candidates) + .find(|path| path.exists()) } fn run_udevd() -> Option> { From 6f25e5617d5ee90b81b879d0be2a6998eaac3956 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 11:31:08 +0100 Subject: [PATCH 27/30] Allow run_cmd() with non-utf8-strings The argument to Command::new() in the Rust standard library is anything that implements AsRef. However, the wrapper function run_cmd() so far only allows &str. This meant that run_udevd() had to use to_string_lossy() to turn a PathBuf into a String that could be passed to this function. This commit changes run_cmd() to accept an AsRef so that the call to to_string_lossy() is no longer necessary. The implementation then could no longer use format!() with {} to display the value since OsStr does not implement Display. Thus, I switched this to use the Debug formatting, but I did not actually check how that looks like. This change then triggered a clippy warning that I am fixing in this commit as well: warning: the borrowed expression implements the required traits --> src/main.rs:813:32 | 813 | utils::run_cmd(&format!("{}/virtme-snapd-script", guest_tools_dir), &[]); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `format!("{}/virtme-snapd-script", guest_tools_dir)` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow = note: `#[warn(clippy::needless_borrow)]` on by default Signed-off-by: Uli Schlachter --- src/main.rs | 4 ++-- src/utils.rs | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/main.rs b/src/main.rs index 7e7ec8b..a0aa5a9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -482,7 +482,7 @@ fn run_udevd() -> Option> { let handle = thread::spawn(move || { disable_uevent_helper(); let args: &[&str] = &["--daemon", "--resolve-names=never"]; - utils::run_cmd(&udevd_path.to_string_lossy(), args); + utils::run_cmd(udevd_path, args); utils::log("triggering udev coldplug"); utils::run_cmd("udevadm", &["trigger", "--type=subsystems", "--action=add"]); utils::run_cmd("udevadm", &["trigger", "--type=devices", "--action=add"]); @@ -810,7 +810,7 @@ fn run_snapd() { return; } if let Some(guest_tools_dir) = get_guest_tools_dir() { - utils::run_cmd(&format!("{}/virtme-snapd-script", guest_tools_dir), &[]); + utils::run_cmd(format!("{}/virtme-snapd-script", guest_tools_dir), &[]); } Command::new(snapd_bin) .stdin(Stdio::null()) diff --git a/src/utils.rs b/src/utils.rs index a9aa976..46b5a17 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -7,7 +7,7 @@ use nix::mount::{mount, MsFlags}; use nix::sys::stat::Mode; use nix::unistd::{chown, Gid, Uid}; -use std::ffi::CString; +use std::ffi::{CString, OsStr}; use std::fs::{File, OpenOptions}; use std::io::{self, Write}; use std::os::unix::fs; @@ -111,8 +111,8 @@ pub fn do_mount(source: &str, target: &str, fstype: &str, flags: usize, fsdata: } } -pub fn run_cmd(cmd: &str, args: &[&str]) { - let output = Command::new(cmd) +pub fn run_cmd(cmd: impl AsRef, args: &[&str]) { + let output = Command::new(&cmd) .args(args) .stdin(Stdio::null()) .stdout(Stdio::piped()) @@ -127,8 +127,8 @@ pub fn run_cmd(cmd: &str, args: &[&str]) { } Err(_) => { log(&format!( - "WARNING: failed to run: {} {}", - cmd, + "WARNING: failed to run: {:?} {}", + cmd.as_ref(), args.join(" ") )); } From 85a942eeceefafd52ade96375ba52891a03581b9 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 11:48:11 +0100 Subject: [PATCH 28/30] Avoid allocations for static strings for run_shell() So far, the code calling run_shell() constructed a Vec just because one of the entries needed to be a dynamic string. This commit changes that to be a Vec<&str>. The one dynamic argument is allocated to another variable and then just borrowed. This allows to get rid of some to_owned() calls, which IMO make the code more readable. Then, run_shell() does not actually need a Vec as its argument, so this is changed into a slice that can then also be passed directly to Commands::args(). Signed-off-by: Uli Schlachter --- src/main.rs | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/main.rs b/src/main.rs index a0aa5a9..d5f689b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -695,10 +695,10 @@ fn detach_from_terminal(tty_fd: libc::c_int) { } } -fn run_shell(tty_fd: libc::c_int, args: Vec) { +fn run_shell(tty_fd: libc::c_int, args: &[&str]) { unsafe { Command::new("bash") - .args(args.into_iter()) + .args(args) .pre_exec(move || { detach_from_terminal(tty_fd); Ok(()) @@ -745,27 +745,31 @@ fn run_user_gui(tty_fd: libc::c_int) { } // Run graphical app using xinit directly - let mut args: Vec = vec!["-l".to_owned(), "-c".to_owned()]; + let mut args = vec!["-l", "-c"]; + let storage; if let Ok(user) = env::var("virtme_user") { // Try to fix permissions on the virtual consoles, we are starting X // directly here so we may need extra permissions on the tty devices. utils::run_cmd("bash", &["-c", &format!("chown {} /dev/char/*", user)]); // Start xinit directly. - args.push(format!("su {} -c 'xinit /tmp/.xinitrc'", user)); + storage = format!("su {} -c 'xinit /tmp/.xinitrc'", user); + args.push(&storage); } else { - args.push("xinit /tmp/.xinitrc".to_owned()); + args.push("xinit /tmp/.xinitrc"); } - run_shell(tty_fd, args); + run_shell(tty_fd, &args); } fn run_user_shell(tty_fd: libc::c_int) { - let mut args: Vec = vec!["-l".to_owned()]; + let mut args = vec!["-l"]; + let storage; if let Ok(user) = env::var("virtme_user") { - args.push("-c".to_owned()); - args.push(format!("su {}", user)); + args.push("-c"); + storage = format!("su {}", user); + args.push(&storage); } - run_shell(tty_fd, args); + run_shell(tty_fd, &args); } fn run_user_session() { From 8cca65ecd9b6034ab5bb51696ee322a5585bf688 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 11:50:57 +0100 Subject: [PATCH 29/30] Use nix' flags instead of libc's in one more place Signed-off-by: Uli Schlachter --- src/main.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/main.rs b/src/main.rs index d5f689b..92a7191 100644 --- a/src/main.rs +++ b/src/main.rs @@ -783,10 +783,9 @@ fn run_user_session() { }; configure_terminal(consdev.as_str()); - let flags = libc::O_RDWR | libc::O_NONBLOCK; + let flags = OFlag::O_RDWR | OFlag::O_NONBLOCK; let mode = Mode::empty(); - let tty_fd = open(consdev.as_str(), OFlag::from_bits_truncate(flags), mode) - .expect("failed to open console"); + let tty_fd = open(consdev.as_str(), flags, mode).expect("failed to open console"); if env::var("virtme_graphics").is_ok() { run_user_gui(tty_fd); From 6c5e94d37c70c7daba1d2199d3404a3c97b89bcf Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Nov 2023 13:05:41 +0100 Subject: [PATCH 30/30] Avoid temporary allocation for PathBuf PathBuf is to Path what String is to str. This code here only needs a Path since join() allocates a new PathBuf. Thus, this gets rid of a temporary allocation for the original PathBuf. Signed-off-by: Uli Schlachter --- src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.rs b/src/main.rs index 92a7191..5ecce47 100644 --- a/src/main.rs +++ b/src/main.rs @@ -469,7 +469,7 @@ fn find_udevd() -> Option { PathBuf::from("/lib/systemd/systemd-udevd"), ]; let path = env::var("PATH").unwrap_or_else(|_| String::new()); - let path_candidates = path.split(':').map(|dir| PathBuf::from(dir).join("udevd")); + let path_candidates = path.split(':').map(|dir| Path::new(dir).join("udevd")); static_candidates .into_iter()