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 0A6F61FF37F for ; Thu, 18 Apr 2024 10:48:17 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id F01CA1760B; Thu, 18 Apr 2024 10:48:12 +0200 (CEST) Date: Thu, 18 Apr 2024 10:48:09 +0200 From: Christoph Heiss To: Aaron Lauterer Message-ID: References: <20240417123108.212720-1-a.lauterer@proxmox.com> <20240417123108.212720-37-a.lauterer@proxmox.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240417123108.212720-37-a.lauterer@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.000 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com] 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-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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. [..] > > +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) > + 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()) > + 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()), } } > + Ok(_) => (), > + Err(_) => bail!("Could not find 'xorriso'. Please install it and try again"), > + } > + > + match Path::try_exists(&args.source) { > + Ok(true) => (), > + Ok(false) => bail!("Source file does not exist."), > + Err(_) => bail!("Source file does not exist."), > + } > + > + match Command::new("xorriso") > + .arg("-dev") > + .arg(&args.source) > + .arg("-find") > + .arg(PROXMOX_ISO_FLAG) > + .stderr(Stdio::null()) > + .stdout(Stdio::null()) > + .status() > + { > + Ok(v) => { > + if !v.success() { > + bail!("The source ISO file is not able to be installed automatically. Please try a more current one."); > + } > + } > + Err(_) => bail!("Could not run 'xorriso'. Please install it."), > + }; > + > + Ok(()) > +} > -- > 2.39.2 > > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel