Re: [PATCH installer] assistant: add support for splitting ISO into (i)PXE-compatible files

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 04.02.26 um 13:09 schrieb Christoph Heiss:
> Adds a new flag to the `prepare-iso` subcommand; `--pxe`.
>
> When specified, instead of just creating the usual
> auto-installation-capable ISO, additionally `vmlinuz` and `initrd.img`
> will be created, which can be used in combination with the ISO to
> PXE-boot our installer.

Nice feature, some comments inline.

> diff --git a/proxmox-auto-install-assistant/src/main.rs b/proxmox-auto-install-assistant/src/main.rs
> [...]
> +impl FromStr for PxeLoader {
> +    type Err = anyhow::Error;
> +
> +    fn from_str(s: &str) -> Result<Self> {
> +        match s {
> +            "none" => Ok(PxeLoader::None),
> +            "ipxe" => Ok(PxeLoader::Ipxe),
> +            _ => bail!("unknown PXE loader '{s}'"),
> +        }
> +    }
> +}

> +      --pxe [<TYPE>]
> +          Instead of producing an ISO file, generate a 'initrd.img' and 'vmlinuz' file for use with
> +          (i)PXE servers. The '--output' option must point to a directory to place these files in.
> +
> +          <TYPE> is optional. Possible value are 'syslinux', 'ipxe'. If <TYPE> is specified, a
> +          configuration file is produced for the specified loader.

The help text lists 'syslinux' as a possible value but FromStr doesn't
handle it, so passing --pxe syslinux gives a confusing "unknown PXE
loader 'syslinux'" error, let's drop that for now if it's currently not
planned.

Also, small grammar nit: "Possible value are" -> "Possible values are"

> +    fn parse(args: &mut cli::Arguments) -> Result<Self> {
> +        let pxe_loader = match args.opt_value_from_str("--pxe") {
> +            Ok(val) => val,
> +            Err(cli::Error::OptionWithoutAValue(_)) => Some(PxeLoader::default()),
> +            Err(err) => Err(err)?,
> +        };

This has a subtle interaction with pico_args' optional value handling:
if --pxe directly precedes the positional input argument (e.g.
`prepare-iso --fetch-from http --pxe input.iso`), pico_args will try
to parse "input.iso" as a PxeLoader and fail with "unknown PXE loader
'input.iso'". Only works when --pxe is followed by another --flag or
is given an explicit type value.

Not a blocker since the same applies to other optional-value patterns
with pico_args, but might be worth a note in the help text (e.g.
"use '--pxe none' when no other flags follow"). Alternatively, use
--pxe as flag and --pxe-type <type> where the value is not optional.

> +    if args.pxe_loader.is_some()
> +        && let Some(out) = &args.output
> +        && (!fs::exists(out)? || !fs::metadata(out)?.is_dir())
> +    {
> +        bail!("'--output' must point to a directory when '--pxe' is specified.");
> +    }

This check silently passes when --pxe is given without --output, since
the `let Some(out)` guard skips it. The actual error only surfaces later
inside final_iso_location() with a rather generic message. I'd add an
early dedicated check here, something like:

  if args.pxe_loader.is_some() && args.output.is_none() {
      bail!("'--output' must be specified when '--pxe' is used.");
  }

> +fn prepare_pxe_compatible_files(
> +    args: &CommandPrepareISOArgs,
> [...]
> +    let out_dir = match &args.output {
> +        Some(out) => out,
> +        None => &args
> +            .input
> +            .parent()
> +            .ok_or_else(|| anyhow!("valid parent path"))?
> +            .to_path_buf(),
> +    };

Two things here:

1) The None branch is dead code - prepare_pxe_compatible_files() is only
   called when pxe_loader.is_some(), and final_iso_location() (called
   earlier) already bails if output is None in PXE mode. If --output is
   actually meant to be optional in PXE mode (falling back to the input
   directory), then final_iso_location would need adjusting instead.

2) The error message `anyhow!("valid parent path")` reads as a sentence
   fragment, not an error. Something like "input path has no parent
   directory" would be clearer, but given (1) it's (currently) moot.


> diff --git a/proxmox-installer-common/src/setup.rs b/proxmox-installer-common/src/setup.rs
> [...]
> +    pub fn full_name(&self) -> &'static str {
> +        match self {
> +            Self::PVE => "Proxmox Virtual Environment",
> +            Self::PMG => "Proxmox Mail Gateway",
> +            Self::PBS => "Proxmox Backup Server",
> +            Self::PDM => "Proxmox Datacenter Manager",
> +        }
> +    }

Minor: This is the third way to get a product display name (Display
gives the short lowercase form, ProductConfig::fullname the "Proxmox
VE"-style form, and now this one the full unabbreviated form). Fine for
now, but worth keeping in mind and not lets add yet another one in the
future ^^

Apart from the above, the overall approach looks good to me.