Some refactorings in the hope this improves the code #2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 <psychon@znc.in>
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 <psychon@znc.in>
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 <psychon@znc.in>
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 <psychon@znc.in>
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 <psychon@znc.in>
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 <psychon@znc.in>
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 <psychon@znc.in>
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 <psychon@znc.in>
"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 <psychon@znc.in>
Clippy reports that there is code here that takes an Option<String> and
produces the exact same Option<String> 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 <psychon@znc.in>
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 <psychon@znc.in>
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 <psychon@znc.in>
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 <psychon@znc.in>
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 <psychon@znc.in>
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 <psychon@znc.in>
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 <psychon@znc.in>
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 <psychon@znc.in>
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<Option<thread::JoinHandle<()>>> = 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<Option<thread::JoinHandle<()>>> = 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 <psychon@znc.in>
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 <psychon@znc.in>
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 <psychon@znc.in>
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 <psychon@znc.in>
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 <psychon@znc.in>
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 <psychon@znc.in>
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 <psychon@znc.in>
An array is just as good as a Vec here and avoids a memory allocation. Signed-off-by: Uli Schlachter <psychon@znc.in>
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 <psychon@znc.in>
The argument to Command::new() in the Rust standard library is anything
that implements AsRef<OsStr>. 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<OsStr> 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 <psychon@znc.in>
So far, the code calling run_shell() constructed a Vec<String> 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 <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
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 <psychon@znc.in>
|
@psychon reviewed, tested and merged all! Everything looks good to me and the code is definitely much cleaner with your changes. I also appreciate the fact that you didn't introduce additional dependencies. Thanks! :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi,
first: Testing done: None. Sorry.
I came across the LWN article, read "Rust" and was curious. While reading the code, I had an idea for something that might be a simplification. Then another one. At some point, I started looking at
cargo clippyand incorporated its suggestions. Then I had some other ideas. This PR is the result.The idea is to only do refactorings. I didn't change anything related to e.g. error handling, so the behaviour should still be exactly the same as before. But again, I didn't do any testing (except for the one refactoring where I added a unit test).
Feel free to reject all of this. Feel free to cherry-pick the things you like. Feel free to tell me to rework things, split this up, rewrite history. I am not trying to cause work for you, but only did this out of fun.
I also wanted to change the
log()function, but didn't come up with any easy and obvious versions and didn't want to dump a macro-mess on you, nor a new dependency like thelogortracingcrates.