[pve-devel] [PATCH installer 2/7] tui: improve `FormView` error handling
Christoph Heiss
c.heiss at proxmox.com
Wed Oct 4 16:42:13 CEST 2023
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 <c.heiss at proxmox.com>
---
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::<EditView, _>(0)
- .ok_or("failed to retrieve password")?;
+ .map_err(|_| "failed to retrieve password")?;
let confirm_password = view
.get_value::<EditView, _>(1)
- .ok_or("failed to retrieve password confirmation")?;
+ .map_err(|_| "failed to retrieve password confirmation")?;
let email = view
.get_value::<EditView, _>(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::<SelectView, _>(0)
- .ok_or("failed to retrieve management interface name")?;
+ .map_err(|_| "failed to retrieve management interface name")?;
let fqdn = view
.get_value::<EditView, _>(1)
- .ok_or("failed to retrieve host FQDN")?
+ .map_err(|_| "failed to retrieve host FQDN")?
.parse::<Fqdn>()
.map_err(|err| format!("hostname does not look valid:\n\n{err}"))?;
let address = view
.get_value::<CidrAddressEditView, _>(2)
- .ok_or("failed to retrieve host address")?;
+ .map_err(|_| "failed to retrieve host address")?;
let gateway = view
.get_value::<EditView, _>(3)
- .ok_or("failed to retrieve gateway address")?
+ .map_err(|_| "failed to retrieve gateway address")?
.parse::<IpAddr>()
.map_err(|err| err.to_string())?;
let dns_server = view
.get_value::<EditView, _>(4)
- .ok_or("failed to retrieve DNS server address")?
+ .map_err(|_| "failed to retrieve DNS server address")?
.parse::<IpAddr>()
.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::<NamedView<FormView>>())
.map(NamedView::<FormView>::get_mut)
- .and_then(|v| v.get_value::<SelectView<Disk>, _>(0))
- .ok_or("failed to retrieve bootdisk")?;
+ .ok_or("failed go retrieve disk selection view")
+ .and_then(|v| {
+ v.get_value::<SelectView<Disk>, _>(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::<FormView>())
- .and_then(|v| v.get_value::<SelectView<FsType>, _>(0))
- .ok_or("Failed to retrieve filesystem type".to_owned())?;
+ .ok_or("Failed to retrieve advanced bootdisk options view")?
+ .get_value::<SelectView<FsType>, _>(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::<LvmBootdiskOptionsView>() {
- 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::<ZfsBootdiskOptionsView>() {
- 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::<BtrfsBootdiskOptionsView>() {
- 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<LvmBootdiskOptions> {
+ fn get_values(&mut self) -> Result<LvmBootdiskOptions, String> {
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::<DiskSizeEditView, _>(0)?;
+ let swap_size = self.view.get_opt_value::<DiskSizeEditView, _>(1)?;
+
let max_root_size = if is_pve {
- self.view.get_value::<DiskSizeEditView, _>(2)
+ self.view.get_opt_value::<DiskSizeEditView, _>(2)?
} else {
None
};
let max_data_size = if is_pve {
- self.view.get_value::<DiskSizeEditView, _>(3)
+ self.view.get_opt_value::<DiskSizeEditView, _>(3)?
} else {
None
};
- Some(LvmBootdiskOptions {
- total_size: self.view.get_value::<DiskSizeEditView, _>(0)?,
- swap_size: self.view.get_value::<DiskSizeEditView, _>(1),
+
+ let min_lvm_free = self
+ .view
+ .get_opt_value::<DiskSizeEditView, _>(min_lvm_free_id)?;
+
+ Ok(LvmBootdiskOptions {
+ total_size,
+ swap_size,
max_root_size,
max_data_size,
- min_lvm_free: self.view.get_value::<DiskSizeEditView, _>(min_lvm_free_id),
+ min_lvm_free,
})
}
}
@@ -405,7 +410,7 @@ impl<T: View> MultiDiskOptionsView<T> {
let mut selected_disks = Vec::new();
for i in 0..disk_form.len() {
- let disk = disk_form.get_value::<SelectView<Option<Disk>>, _>(i)?;
+ let disk = disk_form.get_value::<SelectView<Option<Disk>>, _>(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<Disk>, BtrfsBootdiskOptions)> {
- let (disks, selected_disks) = self.view.get_disks_and_selection()?;
- let disk_size = self.view.inner_mut()?.get_value::<DiskSizeEditView, _>(0)?;
+ fn get_values(&mut self) -> Result<(Vec<Disk>, 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::<DiskSizeEditView, _>(0)?;
- Some((
+ Ok((
disks,
BtrfsBootdiskOptions {
disk_size,
@@ -524,17 +537,31 @@ impl ZfsBootdiskOptionsView {
Self { view }
}
- fn get_values(&mut self) -> Option<(Vec<Disk>, ZfsBootdiskOptions)> {
- let (disks, selected_disks) = self.view.get_disks_and_selection()?;
- let view = self.view.inner_mut()?;
-
- let ashift = view.get_value::<IntegerEditView, _>(0)?;
- let compress = view.get_value::<SelectView<_>, _>(1)?;
- let checksum = view.get_value::<SelectView<_>, _>(2)?;
- let copies = view.get_value::<IntegerEditView, _>(3)?;
- let disk_size = view.get_value::<DiskSizeEditView, _>(4)?;
-
- Some((
+ fn get_values(&mut self) -> Result<(Vec<Disk>, 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::<IntegerEditView, _>(0)
+ .map_err(|err| format!("invalid ashift value: {err}"))?;
+ let compress = view
+ .get_value::<SelectView<_>, _>(1)
+ .map_err(|_| "failed to get compress option")?;
+ let checksum = view
+ .get_value::<SelectView<_>, _>(2)
+ .map_err(|_| "failed to get checksum option")?;
+ let copies = view
+ .get_value::<IntegerEditView, _>(3)
+ .map_err(|err| format!("invalid copies value: {err}"))?;
+ let disk_size = view
+ .get_value::<DiskSizeEditView, _>(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<T>
+where
+ T: FromStr,
+{
+ ParseError(<T as FromStr>::Err),
+ OutOfRange {
+ value: T,
+ min: Option<T>,
+ max: Option<T>,
+ },
+ Empty,
+ InvalidView,
+}
+
+impl<T> fmt::Display for NumericEditViewError<T>
+where
+ T: fmt::Display + FromStr,
+ <T as 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<T> {
view: EditView,
max_value: Option<T>,
@@ -25,7 +65,10 @@ pub struct NumericEditView<T> {
allow_empty: bool,
}
-impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
+impl<T> NumericEditView<T>
+where
+ T: Copy + ToString + FromStr + PartialOrd,
+{
pub fn new() -> Self {
Self {
view: EditView::new().content("0"),
@@ -35,6 +78,15 @@ impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
}
}
+ 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<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
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<T, <T as FromStr>::Err> {
- assert!(!self.allow_empty);
- self.view.get_content().parse()
- }
-
- pub fn get_content_maybe(&self) -> Option<Result<T, <T as FromStr>::Err>> {
+ pub fn get_content(&self) -> Result<T, NumericEditViewError<T>> {
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<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
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<String>, 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<f64> {
- self.with_view(|v| {
- v.get_child(0)?
- .downcast_ref::<ResizedView<FloatEditView>>()?
- .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<f64, NumericEditViewError<f64>> {
+ let content = self
+ .with_view(|v| {
+ v.get_child(0)?
+ .downcast_ref::<ResizedView<FloatEditView>>()?
+ .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<R> {
- fn get_value(&self) -> Option<R>;
+ type Error;
+
+ fn get_value(&self) -> Result<R, Self::Error>;
+
+ fn get_opt_value(&self) -> Result<Option<R>, Self::Error> {
+ self.get_value().map(|val| Some(val))
+ }
}
impl FormViewGetValue<String> for EditView {
- fn get_value(&self) -> Option<String> {
- Some((*self.get_content()).clone())
+ type Error = ();
+
+ fn get_value(&self) -> Result<String, Self::Error> {
+ Ok((*self.get_content()).clone())
}
}
impl<T: 'static + Clone> FormViewGetValue<T> for SelectView<T> {
- fn get_value(&self) -> Option<T> {
- self.selection().map(|v| (*v).clone())
+ type Error = ();
+
+ fn get_value(&self) -> Result<T, Self::Error> {
+ self.selection().map(|v| (*v).clone()).ok_or(())
}
}
@@ -272,14 +318,26 @@ where
T: Copy + ToString + FromStr + PartialOrd,
NumericEditView<T>: ViewWrapper,
{
- fn get_value(&self) -> Option<T> {
- self.get_content().ok()
+ type Error = NumericEditViewError<T>;
+
+ fn get_value(&self) -> Result<T, Self::Error> {
+ self.get_content()
+ }
+
+ fn get_opt_value(&self) -> Result<Option<T>, Self::Error> {
+ match self.get_content() {
+ Ok(val) => Ok(Some(val)),
+ Err(NumericEditViewError::Empty) => Ok(None),
+ Err(err) => Err(err),
+ }
}
}
impl FormViewGetValue<CidrAddress> for CidrAddressEditView {
- fn get_value(&self) -> Option<CidrAddress> {
- self.get_values()
+ type Error = ();
+
+ fn get_value(&self) -> Result<CidrAddress, Self::Error> {
+ self.get_values().ok_or(())
}
}
@@ -289,15 +347,59 @@ where
NamedView<T>: ViewWrapper,
<NamedView<T> as ViewWrapper>::V: FormViewGetValue<R>,
{
- fn get_value(&self) -> Option<R> {
- self.with_view(|v| v.get_value()).flatten()
+ type Error = ();
+
+ fn get_value(&self) -> Result<R, Self::Error> {
+ match self.with_view(|v| v.get_value()) {
+ Some(Ok(res)) => Ok(res),
+ _ => Err(()),
+ }
}
}
impl FormViewGetValue<f64> for DiskSizeEditView {
- fn get_value(&self) -> Option<f64> {
+ type Error = NumericEditViewError<f64>;
+
+ fn get_value(&self) -> Result<f64, Self::Error> {
self.get_content()
}
+
+ fn get_opt_value(&self) -> Result<Option<f64>, Self::Error> {
+ match self.get_content() {
+ Ok(val) => Ok(Some(val)),
+ Err(NumericEditViewError::Empty) => Ok(None),
+ Err(err) => Err(err),
+ }
+ }
+}
+
+pub enum FormViewError<T> {
+ ChildNotFound(usize),
+ ValueError(T),
+}
+
+impl<T: fmt::Display> fmt::Display for FormViewError<T> {
+ 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<T: fmt::Debug> fmt::Debug for FormViewError<T> {
+ 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<T: fmt::Display> From<FormViewError<T>> for String {
+ fn from(value: FormViewError<T>) -> Self {
+ value.to_string()
+ }
}
pub struct FormView {
@@ -348,11 +450,28 @@ impl FormView {
.downcast_mut::<T>()
}
- pub fn get_value<T, R>(&self, index: usize) -> Option<R>
+ pub fn get_value<T, R>(
+ &self,
+ index: usize,
+ ) -> Result<R, FormViewError<<T as FormViewGetValue<R>>::Error>>
+ where
+ T: View + FormViewGetValue<R>,
+ {
+ self.get_child::<T>(index)
+ .ok_or(FormViewError::ChildNotFound(index))
+ .and_then(|v| v.get_value().map_err(FormViewError::ValueError))
+ }
+
+ pub fn get_opt_value<T, R>(
+ &self,
+ index: usize,
+ ) -> Result<Option<R>, FormViewError<<T as FormViewGetValue<R>>::Error>>
where
T: View + FormViewGetValue<R>,
{
- self.get_child::<T>(index)?.get_value()
+ self.get_child::<T>(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::<SelectView, _>(0)
- .ok_or("failed to retrieve timezone")?;
+ .map_err(|_| "failed to retrieve country")?;
let timezone = self
.view
.get_value::<NamedView<SelectView>, _>(1)
- .ok_or("failed to retrieve timezone")?;
+ .map_err(|_| "failed to retrieve timezone")?;
let kmap = self
.view
.get_value::<SelectView<KeyboardMapping>, _>(2)
- .ok_or("failed to retrieve keyboard layout")?;
+ .map_err(|_| "failed to retrieve keyboard layout")?;
Ok(TimezoneOptions {
country,
--
2.42.0
More information about the pve-devel
mailing list