Re: [pve-devel] [PATCH installer v6 36/36] autoinst-helper: add prepare-iso subcommand

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

Just quick three notes inline; nits other than the crate thing.
Did not review in depth, LGTM overall tho.

On Wed, Apr 17, 2024 at 02:31:08PM +0200, Aaron Lauterer wrote:
[..]
> diff --git a/proxmox-autoinst-helper/Cargo.toml b/proxmox-autoinst-helper/Cargo.toml
> index 2a88c0f..75399e0 100644
> --- a/proxmox-autoinst-helper/Cargo.toml
> +++ b/proxmox-autoinst-helper/Cargo.toml
> @@ -19,3 +19,4 @@ serde_json = "1.0"
>  toml = "0.7"
>  log = "0.4.20"
>  regex = "1.7"
> +which = "4.2.5"

Misses the debian/control entry, but see also below.

[..]
>
> +fn prepare_iso(args: &CommandPrepareISO) -> Result<()> {
> +    check_prepare_requirements(args)?;
> +
> +    if args.install_mode == AutoInstModes::Included && args.answer_file.is_none() {
> +        bail!("Missing path to answer file needed for 'direct' install mode.");
> +    }
> +    if args.install_mode == AutoInstModes::Included && args.cert_fingerprint.is_some() {
> +        bail!("No certificate fingerprint needed for direct install mode. Drop the parameter!");
> +    }
> +    if args.install_mode == AutoInstModes::Included && args.url.is_some() {
> +        bail!("No URL needed for direct install mode. Drop the parameter!");
> +    }

  if args.install_mode == AutoInstModes::Included {
    if args.answer_file.is_none() {
        bail!("Missing path to answer file needed for 'direct' install mode.");
    }
    if args.cert_fingerprint.is_some() {
        bail!("No certificate fingerprint needed for direct install mode. Drop the parameter!");
    }
    if args.url.is_some() {
        bail!("No URL needed for direct install mode. Drop the parameter!");
    }
  } else if (args.install_mode == AutoInstModes::Partition) { .. }

.. maybe, to avoid the repeated condition? (The resulting visual
grouping is also nice)


> +    if args.answer_file.is_some() && args.install_mode != AutoInstModes::Included {
> +        bail!("Set '-i', '--install-mode' to 'included' to place the answer file directly in the ISO.");
> +    }
> +    if args.install_mode == AutoInstModes::Partition && args.cert_fingerprint.is_some() {
> +        bail!("No certificate fingerprint needed for partition install mode. Drop the parameter!");
> +    }
> +    if args.install_mode == AutoInstModes::Partition && args.url.is_some() {
> +        bail!("No URL needed for partition install mode. Drop the parameter!");
> +    }
> +
[..]
> +
>  fn get_disks() -> Result<BTreeMap<String, BTreeMap<String, String>>> {
>      let unwantend_block_devs = vec![
>          "ram[0-9]*",
> @@ -335,3 +510,53 @@ fn get_udev_properties(path: &PathBuf) -> Result<String> {
>      }
>      Ok(String::from_utf8(udev_output.stdout)?)
>  }
> +
> +fn parse_answer(path: &PathBuf) -> Result<Answer> {
> +    let mut file = match fs::File::open(path) {
> +        Ok(file) => file,
> +        Err(err) => bail!("Opening answer file '{}' failed: {err}", path.display()),
> +    };
> +    let mut contents = String::new();
> +    if let Err(err) = file.read_to_string(&mut contents) {
> +        bail!("Reading from file '{}' failed: {err}", path.display());
> +    }

There is also std::fs::read_to_string() for exactly that; and would
avoid the whole open/close dance :^)

(Seems I missed that when reviewing the patch that introduced
validate_answer())

> +    match toml::from_str(&contents) {
> +        Ok(answer) => {
> +            println!("The file was parsed successfully, no syntax errors found!");
> +            Ok(answer)
> +        }
> +        Err(err) => bail!("Error parsing answer file: {err}"),
> +    }
> +}
> +
> +fn check_prepare_requirements(args: &CommandPrepareISO) -> Result<()> {
> +    match which("xorriso") {

Do we really need _yet another_ crate dependency for that? Below is a
check / bail! anyway when running the command proper.

And if we really want a explicit check beforehand, I'd just do something
like

  fn which(name: &str) -> Result<()> {
      match Command::new(name).output() {
          Ok(_) => Ok(()),
          Err(err) => Err(err.into()),
      }
  }

> +        Ok(_) => (),
> +        Err(_) => bail!("Could not find 'xorriso'. Please install it and try again"),
> +    }
> +
> +    match Path::try_exists(&args.source) {
> +        Ok(true) => (),
> +        Ok(false) => bail!("Source file does not exist."),
> +        Err(_) => bail!("Source file does not exist."),
> +    }
> +
> +    match Command::new("xorriso")
> +        .arg("-dev")
> +        .arg(&args.source)
> +        .arg("-find")
> +        .arg(PROXMOX_ISO_FLAG)
> +        .stderr(Stdio::null())
> +        .stdout(Stdio::null())
> +        .status()
> +    {
> +        Ok(v) => {
> +            if !v.success() {
> +                bail!("The source ISO file is not able to be installed automatically. Please try a more current one.");
> +            }
> +        }
> +        Err(_) => bail!("Could not run 'xorriso'. Please install it."),
> +    };
> +
> +    Ok(())
> +}
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel