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 AEECB1FF137 for ; Tue, 31 Mar 2026 12:30:45 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 10E3C18195; Tue, 31 Mar 2026 12:31:12 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 31 Mar 2026 12:31:05 +0200 Message-Id: From: "Christoph Heiss" To: "Thomas Lamprecht" Subject: Re: [PATCH installer] assistant: add support for splitting ISO into (i)PXE-compatible files X-Mailer: aerc 0.21.0 References: <20260204121025.630269-1-c.heiss@proxmox.com> <4508334a-e1a8-424a-9aee-e49662c1f086@proxmox.com> In-Reply-To: <4508334a-e1a8-424a-9aee-e49662c1f086@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774953010504 X-SPAM-LEVEL: Spam detection results: 0 AWL -1.444 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: TN6P6E5IDA62EENEJRY4TVOSZQNVQCSS X-Message-ID-Hash: TN6P6E5IDA62EENEJRY4TVOSZQNVQCSS X-MailFrom: c.heiss@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 CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Thanks for the review! On Mon Mar 30, 2026 at 6:01 PM CEST, Thomas Lamprecht wrote: [..] >> + --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 directo= ry 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. Yeah, bit of a leftover that slipped through. [..] >> + fn parse(args: &mut cli::Arguments) -> Result { >> + let pxe_loader =3D match args.opt_value_from_str("--pxe") { >> + Ok(val) =3D> val, >> + Err(cli::Error::OptionWithoutAValue(_)) =3D> Some(PxeLoader= ::default()), >> + Err(err) =3D> 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. I'll switch it to the latter, with a separate option. Seems sane(r) than trying to quash everything into one flag. [..] >> diff --git a/proxmox-installer-common/src/setup.rs b/proxmox-installer-c= ommon/src/setup.rs >> [...] >> + pub fn full_name(&self) -> &'static str { >> + match self { >> + Self::PVE =3D> "Proxmox Virtual Environment", >> + Self::PMG =3D> "Proxmox Mail Gateway", >> + Self::PBS =3D> "Proxmox Backup Server", >> + Self::PDM =3D> "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 ^^ Good point. Maybe using `PRODUCT_LONG` here from the cd-info would also be fine too? It's the exact same string; apart from using "Proxmox VE" instead of "Proxmox Virtual Environment". If nothing speaks against that, I'd probably go that route instead, otherwise I'll just keep it like it. (_Really_ bikeshedding now, but it's the first thing users see, so my thought here was to have it in exactly the same spelling as in the GRUB menu of the ISO.)