From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id EB91791BDC for ; Wed, 4 Oct 2023 16:43:16 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 44B81F480 for ; Wed, 4 Oct 2023 16:42:46 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 4 Oct 2023 16:42:44 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id EA5F844833 for ; Wed, 4 Oct 2023 16:42:42 +0200 (CEST) From: Christoph Heiss To: pve-devel@lists.proxmox.com Date: Wed, 4 Oct 2023 16:42:13 +0200 Message-ID: <20231004144232.327071-3-c.heiss@proxmox.com> X-Mailer: git-send-email 2.42.0 In-Reply-To: <20231004144232.327071-1-c.heiss@proxmox.com> References: <20231004144232.327071-1-c.heiss@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.027 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: [pve-devel] [PATCH installer 2/7] tui: improve `FormView` error handling 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: , X-List-Received-Date: Wed, 04 Oct 2023 14:43:17 -0000 Mostly internal changes without any user-visible changes; replaces all optional return values in form with result that can hold more specific error causes. Signed-off-by: Christoph Heiss --- RFC; just a thought: It could make sense to introduce the `anyhow` crate here in the installer as well. We already use it in PBS, so nothing completely new and it would simplify error handling quite a bit as well, I think. But we can also do without it as well, as this series shows. proxmox-tui-installer/src/main.rs | 16 +- proxmox-tui-installer/src/views/bootdisk.rs | 105 +++++---- proxmox-tui-installer/src/views/mod.rs | 245 +++++++++++++++----- proxmox-tui-installer/src/views/timezone.rs | 6 +- 4 files changed, 259 insertions(+), 113 deletions(-) diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs index 3f01713..ab990a8 100644 --- a/proxmox-tui-installer/src/main.rs +++ b/proxmox-tui-installer/src/main.rs @@ -499,15 +499,15 @@ fn password_dialog(siv: &mut Cursive) -> InstallerView { let options = siv.call_on_name("password-options", |view: &mut FormView| { let root_password = view .get_value::(0) - .ok_or("failed to retrieve password")?; + .map_err(|_| "failed to retrieve password")?; let confirm_password = view .get_value::(1) - .ok_or("failed to retrieve password confirmation")?; + .map_err(|_| "failed to retrieve password confirmation")?; let email = view .get_value::(2) - .ok_or("failed to retrieve email")?; + .map_err(|_| "failed to retrieve email")?; let email_regex = Regex::new(r"^[\w\+\-\~]+(\.[\w\+\-\~]+)*@[a-zA-Z0-9\-]+(\.[a-zA-Z0-9\-]+)*$") @@ -588,27 +588,27 @@ fn network_dialog(siv: &mut Cursive) -> InstallerView { let options = siv.call_on_name("network-options", |view: &mut FormView| { let ifname = view .get_value::(0) - .ok_or("failed to retrieve management interface name")?; + .map_err(|_| "failed to retrieve management interface name")?; let fqdn = view .get_value::(1) - .ok_or("failed to retrieve host FQDN")? + .map_err(|_| "failed to retrieve host FQDN")? .parse::() .map_err(|err| format!("hostname does not look valid:\n\n{err}"))?; let address = view .get_value::(2) - .ok_or("failed to retrieve host address")?; + .map_err(|_| "failed to retrieve host address")?; let gateway = view .get_value::(3) - .ok_or("failed to retrieve gateway address")? + .map_err(|_| "failed to retrieve gateway address")? .parse::() .map_err(|err| err.to_string())?; let dns_server = view .get_value::(4) - .ok_or("failed to retrieve DNS server address")? + .map_err(|_| "failed to retrieve DNS server address")? .parse::() .map_err(|err| err.to_string())?; diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs index dbd13ea..46bdd9f 100644 --- a/proxmox-tui-installer/src/views/bootdisk.rs +++ b/proxmox-tui-installer/src/views/bootdisk.rs @@ -75,8 +75,11 @@ impl BootdiskOptionsView { .get_child_mut(0) .and_then(|v| v.downcast_mut::>()) .map(NamedView::::get_mut) - .and_then(|v| v.get_value::, _>(0)) - .ok_or("failed to retrieve bootdisk")?; + .ok_or("failed go retrieve disk selection view") + .and_then(|v| { + v.get_value::, _>(0) + .map_err(|_| "failed to retrieve bootdisk") + })?; options.disks = vec![disk]; } @@ -181,8 +184,9 @@ impl AdvancedBootdiskOptionsView { .view .get_child(1) .and_then(|v| v.downcast_ref::()) - .and_then(|v| v.get_value::, _>(0)) - .ok_or("Failed to retrieve filesystem type".to_owned())?; + .ok_or("Failed to retrieve advanced bootdisk options view")? + .get_value::, _>(0) + .map_err(|_| "Failed to retrieve filesystem type".to_owned())?; let advanced = self .view @@ -190,21 +194,15 @@ impl AdvancedBootdiskOptionsView { .ok_or("Failed to retrieve advanced bootdisk options view".to_owned())?; if let Some(view) = advanced.downcast_mut::() { - let advanced = view - .get_values() - .map(AdvancedBootdiskOptions::Lvm) - .ok_or("Failed to retrieve advanced bootdisk options")?; + let advanced = view.get_values()?; Ok(BootdiskOptions { disks: vec![], fstype, - advanced, + advanced: AdvancedBootdiskOptions::Lvm(advanced), }) } else if let Some(view) = advanced.downcast_mut::() { - let (disks, advanced) = view - .get_values() - .ok_or("Failed to retrieve advanced bootdisk options")?; - + let (disks, advanced) = view.get_values()?; if let FsType::Zfs(level) = fstype { check_zfs_raid_config(level, &disks).map_err(|err| format!("{fstype}: {err}"))?; } @@ -215,9 +213,7 @@ impl AdvancedBootdiskOptionsView { advanced: AdvancedBootdiskOptions::Zfs(advanced), }) } else if let Some(view) = advanced.downcast_mut::() { - let (disks, advanced) = view - .get_values() - .ok_or("Failed to retrieve advanced bootdisk options")?; + let (disks, advanced) = view.get_values()?; if let FsType::Btrfs(level) = fstype { check_btrfs_raid_config(level, &disks).map_err(|err| format!("{fstype}: {err}"))?; @@ -275,25 +271,34 @@ impl LvmBootdiskOptionsView { Self { view } } - fn get_values(&mut self) -> Option { + fn get_values(&mut self) -> Result { let is_pve = crate::setup_info().config.product == ProxmoxProduct::PVE; let min_lvm_free_id = if is_pve { 4 } else { 2 }; + + let total_size = self.view.get_value::(0)?; + let swap_size = self.view.get_opt_value::(1)?; + let max_root_size = if is_pve { - self.view.get_value::(2) + self.view.get_opt_value::(2)? } else { None }; let max_data_size = if is_pve { - self.view.get_value::(3) + self.view.get_opt_value::(3)? } else { None }; - Some(LvmBootdiskOptions { - total_size: self.view.get_value::(0)?, - swap_size: self.view.get_value::(1), + + let min_lvm_free = self + .view + .get_opt_value::(min_lvm_free_id)?; + + Ok(LvmBootdiskOptions { + total_size, + swap_size, max_root_size, max_data_size, - min_lvm_free: self.view.get_value::(min_lvm_free_id), + min_lvm_free, }) } } @@ -405,7 +410,7 @@ impl MultiDiskOptionsView { let mut selected_disks = Vec::new(); for i in 0..disk_form.len() { - let disk = disk_form.get_value::>, _>(i)?; + let disk = disk_form.get_value::>, _>(i).ok()?; // `None` means no disk was selected for this slot if let Some(disk) = disk { @@ -462,11 +467,19 @@ impl BtrfsBootdiskOptionsView { Self { view } } - fn get_values(&mut self) -> Option<(Vec, BtrfsBootdiskOptions)> { - let (disks, selected_disks) = self.view.get_disks_and_selection()?; - let disk_size = self.view.inner_mut()?.get_value::(0)?; + fn get_values(&mut self) -> Result<(Vec, BtrfsBootdiskOptions), String> { + let (disks, selected_disks) = self + .view + .get_disks_and_selection() + .ok_or("failed to retrieve disk selection".to_owned())?; + + let disk_size = self + .view + .inner_mut() + .ok_or("failed to retrieve Btrfs disk size")? + .get_value::(0)?; - Some(( + Ok(( disks, BtrfsBootdiskOptions { disk_size, @@ -524,17 +537,31 @@ impl ZfsBootdiskOptionsView { Self { view } } - fn get_values(&mut self) -> Option<(Vec, ZfsBootdiskOptions)> { - let (disks, selected_disks) = self.view.get_disks_and_selection()?; - let view = self.view.inner_mut()?; - - let ashift = view.get_value::(0)?; - let compress = view.get_value::, _>(1)?; - let checksum = view.get_value::, _>(2)?; - let copies = view.get_value::(3)?; - let disk_size = view.get_value::(4)?; - - Some(( + fn get_values(&mut self) -> Result<(Vec, ZfsBootdiskOptions), String> { + let (disks, selected_disks) = self + .view + .get_disks_and_selection() + .ok_or("failed to retrieve disk selection".to_owned())?; + + let view = self.view.inner_mut().ok_or("failed to retrieve view")?; + + let ashift = view + .get_value::(0) + .map_err(|err| format!("invalid ashift value: {err}"))?; + let compress = view + .get_value::, _>(1) + .map_err(|_| "failed to get compress option")?; + let checksum = view + .get_value::, _>(2) + .map_err(|_| "failed to get checksum option")?; + let copies = view + .get_value::(3) + .map_err(|err| format!("invalid copies value: {err}"))?; + let disk_size = view + .get_value::(4) + .map_err(|err| format!("invalid disk size: {err}"))?; + + Ok(( disks, ZfsBootdiskOptions { ashift, diff --git a/proxmox-tui-installer/src/views/mod.rs b/proxmox-tui-installer/src/views/mod.rs index 76f96a1..7efd487 100644 --- a/proxmox-tui-installer/src/views/mod.rs +++ b/proxmox-tui-installer/src/views/mod.rs @@ -1,4 +1,4 @@ -use std::{mem, net::IpAddr, rc::Rc, str::FromStr}; +use std::{fmt, mem, net::IpAddr, rc::Rc, str::FromStr}; use cursive::{ event::{Event, EventResult}, @@ -18,6 +18,46 @@ pub use table_view::*; mod timezone; pub use timezone::*; +pub enum NumericEditViewError +where + T: FromStr, +{ + ParseError(::Err), + OutOfRange { + value: T, + min: Option, + max: Option, + }, + Empty, + InvalidView, +} + +impl fmt::Display for NumericEditViewError +where + T: fmt::Display + FromStr, + ::Err: fmt::Display, +{ + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use NumericEditViewError::*; + match self { + ParseError(err) => write!(f, "failed to parse value: {err}"), + OutOfRange { value, min, max } => { + write!(f, "out of range: {value}")?; + match (min, max) { + (Some(min), Some(max)) => { + write!(f, ", must be at least {min} and at most {max}") + } + (Some(min), None) => write!(f, ", must be at least {min}"), + (None, Some(max)) => write!(f, ", must be at most {max}"), + _ => Ok(()), + } + } + Empty => write!(f, "value required"), + InvalidView => write!(f, "invalid view state"), + } + } +} + pub struct NumericEditView { view: EditView, max_value: Option, @@ -25,7 +65,10 @@ pub struct NumericEditView { allow_empty: bool, } -impl NumericEditView { +impl NumericEditView +where + T: Copy + ToString + FromStr + PartialOrd, +{ pub fn new() -> Self { Self { view: EditView::new().content("0"), @@ -35,6 +78,15 @@ impl NumericEditView { } } + pub fn new_empty() -> Self { + Self { + view: EditView::new(), + max_value: None, + max_content_width: None, + allow_empty: true, + } + } + pub fn max_value(mut self, max: T) -> Self { self.max_value = Some(max); self @@ -46,30 +98,18 @@ impl NumericEditView { self } - pub fn allow_empty(mut self, value: bool) -> Self { - self.allow_empty = value; - - if value { - self.view = EditView::new(); - } else { - self.view = EditView::new().content("0"); - } - - self.view.set_max_content_width(self.max_content_width); - self - } - - pub fn get_content(&self) -> Result::Err> { - assert!(!self.allow_empty); - self.view.get_content().parse() - } - - pub fn get_content_maybe(&self) -> Option::Err>> { + pub fn get_content(&self) -> Result> { let content = self.view.get_content(); - if !content.is_empty() { - Some(self.view.get_content().parse()) - } else { - None + + match content.parse() { + Err(_) if content.is_empty() && self.allow_empty => Err(NumericEditViewError::Empty), + Err(err) => Err(NumericEditViewError::ParseError(err)), + Ok(value) if !self.in_range(value) => Err(NumericEditViewError::OutOfRange { + value, + min: self.min_value, + max: self.max_value, + }), + Ok(value) => Ok(value), } } @@ -77,6 +117,10 @@ impl NumericEditView { self.max_value = Some(max); } + fn in_range(&self, value: T) -> bool { + !self.max_value.map_or(false, |max| value >= max) + } + fn check_bounds(&mut self, original: Rc, result: EventResult) -> EventResult { // Check if the new value is actually valid according to the max value, if set if let Some(max) = self.max_value { @@ -163,7 +207,6 @@ impl IntegerEditView { pub struct DiskSizeEditView { view: LinearLayout, - allow_empty: bool, } impl DiskSizeEditView { @@ -172,21 +215,15 @@ impl DiskSizeEditView { .child(FloatEditView::new().full_width()) .child(TextView::new(" GB")); - Self { - view, - allow_empty: false, - } + Self { view } } pub fn new_emptyable() -> Self { let view = LinearLayout::horizontal() - .child(FloatEditView::new().allow_empty(true).full_width()) + .child(FloatEditView::new_empty().full_width()) .child(TextView::new(" GB")); - Self { - view, - allow_empty: true, - } + Self { view } } pub fn content(mut self, content: f64) -> Self { @@ -230,20 +267,19 @@ impl DiskSizeEditView { self } - pub fn get_content(&self) -> Option { - self.with_view(|v| { - v.get_child(0)? - .downcast_ref::>()? - .with_view(|v| { - if self.allow_empty { - v.get_content_maybe().and_then(Result::ok) - } else { - v.get_content().ok() - } - }) - .flatten() - }) - .flatten() + pub fn get_content(&self) -> Result> { + let content = self + .with_view(|v| { + v.get_child(0)? + .downcast_ref::>()? + .with_view(|v| v.get_content()) + }) + .flatten(); + + match content { + Some(res) => res, + None => Err(NumericEditViewError::InvalidView), + } } } @@ -252,18 +288,28 @@ impl ViewWrapper for DiskSizeEditView { } pub trait FormViewGetValue { - fn get_value(&self) -> Option; + type Error; + + fn get_value(&self) -> Result; + + fn get_opt_value(&self) -> Result, Self::Error> { + self.get_value().map(|val| Some(val)) + } } impl FormViewGetValue for EditView { - fn get_value(&self) -> Option { - Some((*self.get_content()).clone()) + type Error = (); + + fn get_value(&self) -> Result { + Ok((*self.get_content()).clone()) } } impl FormViewGetValue for SelectView { - fn get_value(&self) -> Option { - self.selection().map(|v| (*v).clone()) + type Error = (); + + fn get_value(&self) -> Result { + self.selection().map(|v| (*v).clone()).ok_or(()) } } @@ -272,14 +318,26 @@ where T: Copy + ToString + FromStr + PartialOrd, NumericEditView: ViewWrapper, { - fn get_value(&self) -> Option { - self.get_content().ok() + type Error = NumericEditViewError; + + fn get_value(&self) -> Result { + self.get_content() + } + + fn get_opt_value(&self) -> Result, Self::Error> { + match self.get_content() { + Ok(val) => Ok(Some(val)), + Err(NumericEditViewError::Empty) => Ok(None), + Err(err) => Err(err), + } } } impl FormViewGetValue for CidrAddressEditView { - fn get_value(&self) -> Option { - self.get_values() + type Error = (); + + fn get_value(&self) -> Result { + self.get_values().ok_or(()) } } @@ -289,15 +347,59 @@ where NamedView: ViewWrapper, as ViewWrapper>::V: FormViewGetValue, { - fn get_value(&self) -> Option { - self.with_view(|v| v.get_value()).flatten() + type Error = (); + + fn get_value(&self) -> Result { + match self.with_view(|v| v.get_value()) { + Some(Ok(res)) => Ok(res), + _ => Err(()), + } } } impl FormViewGetValue for DiskSizeEditView { - fn get_value(&self) -> Option { + type Error = NumericEditViewError; + + fn get_value(&self) -> Result { self.get_content() } + + fn get_opt_value(&self) -> Result, Self::Error> { + match self.get_content() { + Ok(val) => Ok(Some(val)), + Err(NumericEditViewError::Empty) => Ok(None), + Err(err) => Err(err), + } + } +} + +pub enum FormViewError { + ChildNotFound(usize), + ValueError(T), +} + +impl fmt::Display for FormViewError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + Self::ChildNotFound(i) => write!(f, "child with index {i} does not exist"), + Self::ValueError(err) => write!(f, "{err}"), + } + } +} + +impl fmt::Debug for FormViewError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + Self::ChildNotFound(index) => write!(f, "ChildNotFound({index})"), + Self::ValueError(err) => write!(f, "ValueError({err:?})"), + } + } +} + +impl From> for String { + fn from(value: FormViewError) -> Self { + value.to_string() + } } pub struct FormView { @@ -348,11 +450,28 @@ impl FormView { .downcast_mut::() } - pub fn get_value(&self, index: usize) -> Option + pub fn get_value( + &self, + index: usize, + ) -> Result>::Error>> + where + T: View + FormViewGetValue, + { + self.get_child::(index) + .ok_or(FormViewError::ChildNotFound(index)) + .and_then(|v| v.get_value().map_err(FormViewError::ValueError)) + } + + pub fn get_opt_value( + &self, + index: usize, + ) -> Result, FormViewError<>::Error>> where T: View + FormViewGetValue, { - self.get_child::(index)?.get_value() + self.get_child::(index) + .ok_or(FormViewError::ChildNotFound(index)) + .and_then(|v| v.get_opt_value().map_err(FormViewError::ValueError)) } pub fn replace_child(&mut self, index: usize, view: impl View) { diff --git a/proxmox-tui-installer/src/views/timezone.rs b/proxmox-tui-installer/src/views/timezone.rs index 6732286..bd38a92 100644 --- a/proxmox-tui-installer/src/views/timezone.rs +++ b/proxmox-tui-installer/src/views/timezone.rs @@ -100,17 +100,17 @@ impl TimezoneOptionsView { let country = self .view .get_value::(0) - .ok_or("failed to retrieve timezone")?; + .map_err(|_| "failed to retrieve country")?; let timezone = self .view .get_value::, _>(1) - .ok_or("failed to retrieve timezone")?; + .map_err(|_| "failed to retrieve timezone")?; let kmap = self .view .get_value::, _>(2) - .ok_or("failed to retrieve keyboard layout")?; + .map_err(|_| "failed to retrieve keyboard layout")?; Ok(TimezoneOptions { country, -- 2.42.0