From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id C3D461FF141 for ; Mon, 30 Mar 2026 18:01:51 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9B77E4952; Mon, 30 Mar 2026 18:02:17 +0200 (CEST) Message-ID: <4508334a-e1a8-424a-9aee-e49662c1f086@proxmox.com> Date: Mon, 30 Mar 2026 18:01:42 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH installer] assistant: add support for splitting ISO into (i)PXE-compatible files To: Christoph Heiss , pve-devel@lists.proxmox.com References: <20260204121025.630269-1-c.heiss@proxmox.com> Content-Language: en-US From: Thomas Lamprecht In-Reply-To: <20260204121025.630269-1-c.heiss@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774886448295 X-SPAM-LEVEL: Spam detection results: 0 AWL -1.509 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 1 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 1 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 1 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: WMNCRPVPW5AV2SPIQUR4WTTPAO27OTR7 X-Message-ID-Hash: WMNCRPVPW5AV2SPIQUR4WTTPAO27OTR7 X-MailFrom: t.lamprecht@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 { > + match s { > + "none" => Ok(PxeLoader::None), > + "ipxe" => Ok(PxeLoader::Ipxe), > + _ => bail!("unknown PXE loader '{s}'"), > + } > + } > +} > + --pxe [] > + 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. > + > + is optional. Possible value are 'syslinux', 'ipxe'. If 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 { > + 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 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.