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

Thomas Lamprecht
2026-03-30T16:01:51Z

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.