Re: [pve-devel] [PATCH v3 15/30] auto-installer: add fetch answer binary

[Christoph Heiss <c.heiss@proxmox.com>]

Two typos ;) And some small nits

On Thu, Mar 28, 2024 at 02:50:13PM +0100, Aaron Lauterer wrote:
[..]
> diff --git a/proxmox-auto-installer/src/bin/proxmox-fetch-answer.rs b/proxmox-auto-installer/src/bin/proxmox-fetch-answer.rs
> new file mode 100644
> index 0000000..9e89a3c
> --- /dev/null
> +++ b/proxmox-auto-installer/src/bin/proxmox-fetch-answer.rs
[..]
> +fn main() -> ExitCode {
> +    if let Err(err) = init_log() {
> +        panic!("could not initilize logging: {err}");
                             ^^^^^^^^^
typo

And for consistency sake, capitalize the first word here too as done
everywhere else?

> +    }
> +
> +    info!("Fetching answer file");
> +    let answer = match fetch_answer() {
> +        Ok(answer) => answer,
> +        Err(err) => {
> +            error!("Aborting: {}", err);
> +            return ExitCode::FAILURE;
> +        }
> +    };
> +
> +    let mut child = match Command::new("proxmox-auto-installer")
> +        .stdout(Stdio::inherit())
> +        .stdin(Stdio::piped())
> +        .stderr(Stdio::null())
> +        .spawn()
> +    {
> +        Ok(child) => child,
> +        Err(err) => panic!("Failed to start automatic installation: {}", err),

Very much a nit; but above always inline format-strings (i.e. "{err}")
are used, from here on out mostly explicit parameters. Maybe unify them
and use the former consistenly where possible?

E.g. below in `utils.rs` are some more instances

> +    };
> +
> +    let mut stdin = child.stdin.take().expect("Failed to open stdin");
> +    std::thread::spawn(move || {
> +        stdin
> +            .write_all(answer.as_bytes())
> +            .expect("Failed to write to stdin");
> +    });
[..]
> diff --git a/proxmox-auto-installer/src/fetch_plugins/utils.rs b/proxmox-auto-installer/src/fetch_plugins/utils.rs
> new file mode 100644
> index 0000000..82cd3e0
> --- /dev/null
> +++ b/proxmox-auto-installer/src/fetch_plugins/utils.rs
> @@ -0,0 +1,90 @@
> +use anyhow::{bail, Result};
> +use log::{info, warn};
> +use std::{
> +    fs::create_dir_all,
> +    path::{Path, PathBuf},
> +    process::Command,
> +};
> +
> +/// Searches for upper and lower case existence of the partlabel in the search_path
> +///
> +/// # Arguemnts
> +/// * `partlabel_lower` - Partition Label in lower case
> +/// * `search_path` - Path where to search for the partiiton label
> +/// search_path: String

Stray last line?

> +pub fn scan_partlabels(partlabel_lower: &str, search_path: &str) -> Result<PathBuf> {
> +    let partlabel = partlabel_lower.to_uppercase();
> +    let path = Path::new(search_path).join(partlabel.clone());
                                              ^^^^^^^^^^^^^^^^^
Can be `&partlabel`

> +    match path.try_exists() {
> +        Ok(true) => {
> +            info!("Found partition with label '{}'", partlabel);
> +            return Ok(path);
> +        }
> +        Ok(false) => info!("Did not detect partition with label '{}'", partlabel),
> +        Err(err) => info!("Encountered issue, accessing '{}': {}", path.display(), err),
> +    }
> +
> +    let partlabel = partlabel_lower.to_lowercase();

Since you explicitly convert it to lowercase anyway here, I'd say name
`partlabel_lower` does not make much sense (anymore)?

> +    let path = Path::new(search_path).join(partlabel.clone());
                                              ^^^^^^^^^^^^^^^^^
Same here

> +    match path.try_exists() {
> +        Ok(true) => {
> +            info!("Found partition with label '{}'", partlabel);
> +            return Ok(path);
> +        }
> +        Ok(false) => info!("Did not detect partition with label '{}'", partlabel),
> +        Err(err) => info!("Encountered issue, accessing '{}': {}", path.display(), err),
> +    }
> +    bail!(
> +        "Could not detect upper or lower case labels for '{}'",
> +        partlabel_lower
> +    );
> +}
> +
[..]
> +/// Tries to unmount the specified path. Will warn on errors, but not fail.
> +///
> +/// # Arguemnts
> +/// * `target_path` - path to unmount
> +pub fn umount_part(target_path: &str) -> Result<()> {
> +    info!("Unmounting partitiona at {target_path}");
                         ^^^^^^^^^^
typo

> +    match Command::new("umount").arg(target_path).output() {
> +        Ok(output) => {
> +            if output.status.success() {
> +                Ok(())
> +            } else {
> +                warn!("Error unmounting: {}", String::from_utf8(output.stderr)?);
> +                Ok(())
> +            }
> +        }
> +        Err(err) => {
> +            warn!("Error unmounting: {}", err);
> +            Ok(())
> +        }
> +    }
> +}