From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id A4D5B1FF37F for ; Thu, 18 Apr 2024 11:13:57 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 850C717C5C; Thu, 18 Apr 2024 11:13:57 +0200 (CEST) Message-ID: <1abb043b-ccd8-4995-a9f3-5da821b0ffc3@proxmox.com> Date: Thu, 18 Apr 2024 11:13:23 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Christoph Heiss References: <20240417123108.212720-1-a.lauterer@proxmox.com> <20240417123108.212720-37-a.lauterer@proxmox.com> From: Aaron Lauterer In-Reply-To: X-SPAM-LEVEL: Spam detection results: 0 AWL -0.045 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 SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH installer v6 36/36] autoinst-helper: add prepare-iso subcommand X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Cc: Proxmox VE development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On 2024-04-18 10:48, Christoph Heiss wrote: > 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. > thanks for catching that, more further down > [..] >> >> +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) good points > > >> + 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>> { >> let unwantend_block_devs = vec![ >> "ram[0-9]*", >> @@ -335,3 +510,53 @@ fn get_udev_properties(path: &PathBuf) -> Result { >> } >> Ok(String::from_utf8(udev_output.stdout)?) >> } >> + >> +fn parse_answer(path: &PathBuf) -> Result { >> + 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()) ah yeah, I can change that in a follow up > >> + 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()), > } > } I really would like to have an explicit check and nice warning if it isn't present to reduce friction for users. Initially, I thought of doing something like that, but considered it a bit too hacky. But the "do we want an additional crate" argument could be reason enough to switch it over to a test like this. > _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel