* [pve-devel] [PATCH installer v2 2/3] tui: bootdisk: rename `disks` parameter to `avail_disks`
2023-07-25 15:02 [pve-devel] [PATCH installer v2 0/3] fix #4856: tui: bootdisk: use correct defaults in advanced dialog Christoph Heiss
2023-07-25 15:02 ` [pve-devel] [PATCH installer v2 1/3] tui: bootdisk: refactor Rc<RefCell<..>> type into custom type Christoph Heiss
@ 2023-07-25 15:02 ` Christoph Heiss
2023-07-25 15:02 ` [pve-devel] [PATCH installer v2 3/3] fix #4856: tui: bootdisk: use correct defaults in advanced dialog Christoph Heiss
2 siblings, 0 replies; 4+ messages in thread
From: Christoph Heiss @ 2023-07-25 15:02 UTC (permalink / raw)
To: pve-devel
This better conveys its role/contents to the reader, as `disks` might be
ambiguous.
No functional changes.
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
* No changes
proxmox-tui-installer/src/views/bootdisk.rs | 45 ++++++++++++---------
1 file changed, 27 insertions(+), 18 deletions(-)
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 0b0a9f8..d74322a 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -30,13 +30,13 @@ pub struct BootdiskOptionsView {
}
impl BootdiskOptionsView {
- pub fn new(disks: &[Disk], options: &BootdiskOptions) -> Self {
+ pub fn new(avail_disks: &[Disk], options: &BootdiskOptions) -> Self {
let bootdisk_form = FormView::new()
.child(
"Target harddisk",
SelectView::new()
.popup()
- .with_all(disks.iter().map(|d| (d.to_string(), d.clone()))),
+ .with_all(avail_disks.iter().map(|d| (d.to_string(), d.clone()))),
)
.with_name("bootdisk-options-target-disk");
@@ -45,10 +45,10 @@ impl BootdiskOptionsView {
let advanced_button = LinearLayout::horizontal()
.child(DummyView.full_width())
.child(Button::new("Advanced options", {
- let disks = disks.to_owned();
+ let avail_disks = avail_disks.to_owned();
let options = advanced_options.clone();
move |siv| {
- siv.add_layer(advanced_options_view(&disks, options.clone()));
+ siv.add_layer(advanced_options_view(&avail_disks, options.clone()));
}
}));
@@ -91,7 +91,7 @@ struct AdvancedBootdiskOptionsView {
}
impl AdvancedBootdiskOptionsView {
- fn new(disks: &[Disk], options: &BootdiskOptions) -> Self {
+ fn new(avail_disks: &[Disk], options: &BootdiskOptions) -> Self {
let enable_btrfs = crate::setup_info().config.enable_btrfs;
let filter_btrfs = |fstype: &&FsType| -> bool { enable_btrfs || !fstype.is_btrfs() };
@@ -112,8 +112,8 @@ impl AdvancedBootdiskOptionsView {
.unwrap_or_default(),
)
.on_submit({
- let disks = disks.to_owned();
- move |siv, fstype| Self::fstype_on_submit(siv, &disks, fstype)
+ let avail_disks = avail_disks.to_owned();
+ move |siv, fstype| Self::fstype_on_submit(siv, &avail_disks, fstype)
});
let mut view = LinearLayout::vertical()
@@ -124,17 +124,17 @@ impl AdvancedBootdiskOptionsView {
match &options.advanced {
AdvancedBootdiskOptions::Lvm(lvm) => view.add_child(LvmBootdiskOptionsView::new(lvm)),
AdvancedBootdiskOptions::Zfs(zfs) => {
- view.add_child(ZfsBootdiskOptionsView::new(disks, zfs))
+ view.add_child(ZfsBootdiskOptionsView::new(avail_disks, zfs))
}
AdvancedBootdiskOptions::Btrfs(btrfs) => {
- view.add_child(BtrfsBootdiskOptionsView::new(disks, btrfs))
+ view.add_child(BtrfsBootdiskOptionsView::new(avail_disks, btrfs))
}
};
Self { view }
}
- fn fstype_on_submit(siv: &mut Cursive, disks: &[Disk], fstype: &FsType) {
+ fn fstype_on_submit(siv: &mut Cursive, avail_disks: &[Disk], fstype: &FsType) {
siv.call_on_name("advanced-bootdisk-options-dialog", |view: &mut Dialog| {
if let Some(AdvancedBootdiskOptionsView { view }) =
view.get_content_mut().downcast_mut()
@@ -142,15 +142,15 @@ impl AdvancedBootdiskOptionsView {
view.remove_child(3);
match fstype {
FsType::Ext4 | FsType::Xfs => view.add_child(LvmBootdiskOptionsView::new(
- &LvmBootdiskOptions::defaults_from(&disks[0]),
+ &LvmBootdiskOptions::defaults_from(&avail_disks[0]),
)),
FsType::Zfs(_) => view.add_child(ZfsBootdiskOptionsView::new(
- disks,
- &ZfsBootdiskOptions::defaults_from(disks),
+ avail_disks,
+ &ZfsBootdiskOptions::defaults_from(avail_disks),
)),
FsType::Btrfs(_) => view.add_child(BtrfsBootdiskOptionsView::new(
- disks,
- &BtrfsBootdiskOptions::defaults_from(disks),
+ avail_disks,
+ &BtrfsBootdiskOptions::defaults_from(avail_disks),
)),
}
}
@@ -164,7 +164,7 @@ impl AdvancedBootdiskOptionsView {
0,
SelectView::new()
.popup()
- .with_all(disks.iter().map(|d| (d.to_string(), d.clone()))),
+ .with_all(avail_disks.iter().map(|d| (d.to_string(), d.clone()))),
);
}
other => view.replace_child(0, TextView::new(other.to_string())),
@@ -549,9 +549,18 @@ impl ViewWrapper for ZfsBootdiskOptionsView {
cursive::wrap_impl!(self.view: MultiDiskOptionsView<FormView>);
}
-fn advanced_options_view(disks: &[Disk], options_ref: BootdiskOptionsRef) -> impl View {
+/// Creates a dialog containing a [`AdvancedBootdiskOptionsView`] and handling confirmation of the
+/// dialog by saving the selected configuration in `options_ref`.
+///
+/// # Arguments
+///
+/// * `avail_disks` - Available disks for the user to select, with their usage depending on the
+/// selected filesystem type
+/// * `options_ref` - A `BootdiskOptionsRef`, which is used to save the filesystem/RAID
+/// configuration in after confirming the dialog.
+fn advanced_options_view(avail_disks: &[Disk], options_ref: BootdiskOptionsRef) -> impl View {
Dialog::around(AdvancedBootdiskOptionsView::new(
- disks,
+ avail_disks,
&(*options_ref).borrow(),
))
.title("Advanced bootdisk options")
--
2.41.0
^ permalink raw reply [flat|nested] 4+ messages in thread* [pve-devel] [PATCH installer v2 3/3] fix #4856: tui: bootdisk: use correct defaults in advanced dialog
2023-07-25 15:02 [pve-devel] [PATCH installer v2 0/3] fix #4856: tui: bootdisk: use correct defaults in advanced dialog Christoph Heiss
2023-07-25 15:02 ` [pve-devel] [PATCH installer v2 1/3] tui: bootdisk: refactor Rc<RefCell<..>> type into custom type Christoph Heiss
2023-07-25 15:02 ` [pve-devel] [PATCH installer v2 2/3] tui: bootdisk: rename `disks` parameter to `avail_disks` Christoph Heiss
@ 2023-07-25 15:02 ` Christoph Heiss
2 siblings, 0 replies; 4+ messages in thread
From: Christoph Heiss @ 2023-07-25 15:02 UTC (permalink / raw)
To: pve-devel
The size of the install disk was set to the size of the first disk,
regardless of what disk was selected. This only happened if the advanced
options dialog was never opened, and only a disk was selected in the
main bootdisk dialog.
This has quite a bit of churn, but properly solving this involved
restructuring the LVM advanced bootdisk dialog, to also hold the
selected disks, like the ZFS and Btrfs dialogs. In addition to that, the
`BootdiskOptionsRef` needs quite some passing around, to cover all the
cases, since the dialog also needs to be "reentrant-safe".
I tested (among other things):
* Only select disk, don't open the advanced dialog, go to summary,
then back to the bootdisk dialog -> selected disk should be kept
* Select disk, open advanced dialog but leave everything as is, go to
summary, then go back again -> selected disk should be kept
* Same as previous, but change the "Total size" for the disk, go to
summary and back -> selected disk and size should be kept
* Same as previous, but additionally change filesystem to XFS -> disk,
filesystem and size should be kept
* Same as previous, but then create a ZFS RAID, go to summary & back,
ZFS RAID should be kept with all parameters
* etc ..
Further I also verified that the correct disk size(s) get written into
the setup structure for the low-level installer.
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
* No changes
proxmox-tui-installer/src/views/bootdisk.rs | 157 ++++++++++++++------
1 file changed, 108 insertions(+), 49 deletions(-)
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index d74322a..a9e79bc 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -31,17 +31,19 @@ pub struct BootdiskOptionsView {
impl BootdiskOptionsView {
pub fn new(avail_disks: &[Disk], options: &BootdiskOptions) -> Self {
+ let advanced_options = Rc::new(RefCell::new(options.clone()));
+
let bootdisk_form = FormView::new()
.child(
"Target harddisk",
- SelectView::new()
- .popup()
- .with_all(avail_disks.iter().map(|d| (d.to_string(), d.clone()))),
+ target_bootdisk_selectview(
+ avail_disks,
+ advanced_options.clone(),
+ options.disks.first(),
+ ),
)
.with_name("bootdisk-options-target-disk");
- let advanced_options = Rc::new(RefCell::new(options.clone()));
-
let advanced_button = LinearLayout::horizontal()
.child(DummyView.full_width())
.child(Button::new("Advanced options", {
@@ -64,21 +66,10 @@ impl BootdiskOptionsView {
}
pub fn get_values(&mut self) -> Result<BootdiskOptions, String> {
- let mut options = (*self.advanced_options).clone().into_inner();
-
- if [FsType::Ext4, FsType::Xfs].contains(&options.fstype) {
- let disk = self
- .view
- .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")?;
-
- options.disks = vec![disk];
- }
-
- Ok(options)
+ // The simple disk selector, as well as the advanced bootdisk dialog save their
+ // info on submit directly to the shared `BootdiskOptionsRef` - so just clone() + return
+ // it.
+ Ok((*self.advanced_options).clone().into_inner())
}
}
@@ -91,10 +82,10 @@ struct AdvancedBootdiskOptionsView {
}
impl AdvancedBootdiskOptionsView {
- fn new(avail_disks: &[Disk], options: &BootdiskOptions) -> Self {
+ fn new(avail_disks: &[Disk], options_ref: BootdiskOptionsRef) -> Self {
let enable_btrfs = crate::setup_info().config.enable_btrfs;
-
let filter_btrfs = |fstype: &&FsType| -> bool { enable_btrfs || !fstype.is_btrfs() };
+ let options = (*options_ref).borrow();
let fstype_select = SelectView::new()
.popup()
@@ -113,7 +104,10 @@ impl AdvancedBootdiskOptionsView {
)
.on_submit({
let avail_disks = avail_disks.to_owned();
- move |siv, fstype| Self::fstype_on_submit(siv, &avail_disks, fstype)
+ let options_ref = options_ref.clone();
+ move |siv, fstype| {
+ Self::fstype_on_submit(siv, &avail_disks, options_ref.clone(), fstype)
+ }
});
let mut view = LinearLayout::vertical()
@@ -121,8 +115,11 @@ impl AdvancedBootdiskOptionsView {
.child(FormView::new().child("Filesystem", fstype_select))
.child(DummyView.full_width());
+ // Create the appropriate (inner) advanced options view
match &options.advanced {
- AdvancedBootdiskOptions::Lvm(lvm) => view.add_child(LvmBootdiskOptionsView::new(lvm)),
+ AdvancedBootdiskOptions::Lvm(lvm) => {
+ view.add_child(LvmBootdiskOptionsView::new(&options.disks[0], lvm))
+ }
AdvancedBootdiskOptions::Zfs(zfs) => {
view.add_child(ZfsBootdiskOptionsView::new(avail_disks, zfs))
}
@@ -134,16 +131,39 @@ impl AdvancedBootdiskOptionsView {
Self { view }
}
- fn fstype_on_submit(siv: &mut Cursive, avail_disks: &[Disk], fstype: &FsType) {
+ /// Called when a new filesystem type is choosen by the user.
+ /// It first creates the inner (filesystem-specific) options view according to the selected
+ /// filesytem type.
+ /// Further, it replaces the (outer) bootdisk selector in the main dialog, either with a
+ /// selector for LVM configurations or a simple label displaying the chosen RAID for ZFS and
+ /// Btrfs configurations.
+ fn fstype_on_submit(
+ siv: &mut Cursive,
+ avail_disks: &[Disk],
+ options_ref: BootdiskOptionsRef,
+ fstype: &FsType,
+ ) {
+ // Only used for LVM configurations, ZFS and Btrfs do not use the target disk selector
+ let selected_lvm_disk = siv
+ .find_name::<FormView>("bootdisk-options-target-disk")
+ .and_then(|v| v.get_value::<SelectView<Disk>, _>(0));
+
+ // Update the (inner) options view
siv.call_on_name("advanced-bootdisk-options-dialog", |view: &mut Dialog| {
if let Some(AdvancedBootdiskOptionsView { view }) =
view.get_content_mut().downcast_mut()
{
view.remove_child(3);
match fstype {
- FsType::Ext4 | FsType::Xfs => view.add_child(LvmBootdiskOptionsView::new(
- &LvmBootdiskOptions::defaults_from(&avail_disks[0]),
- )),
+ FsType::Ext4 | FsType::Xfs => {
+ // Safety: For LVM setups, the bootdisk SelectView always exists, thus
+ // there will also always be a value.
+ let selected_disk = selected_lvm_disk.clone().unwrap();
+ view.add_child(LvmBootdiskOptionsView::new(
+ &selected_disk,
+ &LvmBootdiskOptions::defaults_from(&selected_disk),
+ ));
+ }
FsType::Zfs(_) => view.add_child(ZfsBootdiskOptionsView::new(
avail_disks,
&ZfsBootdiskOptions::defaults_from(avail_disks),
@@ -156,15 +176,21 @@ impl AdvancedBootdiskOptionsView {
}
});
+ // The "bootdisk-options-target-disk" view might be either a `SelectView` (if ext4 of XFS
+ // is used) or a label containing the filesytem/RAID type (for ZFS and Btrfs).
+ // Now, unconditionally replace it with the appropriate type of these two, depending on the
+ // newly selected filesystem type.
siv.call_on_name(
"bootdisk-options-target-disk",
- |view: &mut FormView| match fstype {
+ move |view: &mut FormView| match fstype {
FsType::Ext4 | FsType::Xfs => {
view.replace_child(
0,
- SelectView::new()
- .popup()
- .with_all(avail_disks.iter().map(|d| (d.to_string(), d.clone()))),
+ target_bootdisk_selectview(
+ avail_disks,
+ options_ref,
+ selected_lvm_disk.as_ref(),
+ ),
);
}
other => view.replace_child(0, TextView::new(other.to_string())),
@@ -186,15 +212,14 @@ impl AdvancedBootdiskOptionsView {
.ok_or("Failed to retrieve advanced bootdisk options view".to_owned())?;
if let Some(view) = advanced.downcast_mut::<LvmBootdiskOptionsView>() {
- let advanced = view
+ let (disk, advanced) = view
.get_values()
- .map(AdvancedBootdiskOptions::Lvm)
.ok_or("Failed to retrieve advanced bootdisk options")?;
Ok(BootdiskOptions {
- disks: vec![],
+ disks: vec![disk],
fstype,
- advanced,
+ advanced: AdvancedBootdiskOptions::Lvm(advanced),
})
} else if let Some(view) = advanced.downcast_mut::<ZfsBootdiskOptionsView>() {
let (disks, advanced) = view
@@ -237,12 +262,13 @@ impl ViewWrapper for AdvancedBootdiskOptionsView {
struct LvmBootdiskOptionsView {
view: FormView,
+ disk: Disk,
}
impl LvmBootdiskOptionsView {
- fn new(options: &LvmBootdiskOptions) -> Self {
+ fn new(disk: &Disk, options: &LvmBootdiskOptions) -> Self {
let is_pve = crate::setup_info().config.product == ProxmoxProduct::PVE;
- // TODO: Set maximum accordingly to disk size
+
let view = FormView::new()
.child(
"Total size",
@@ -269,10 +295,13 @@ impl LvmBootdiskOptionsView {
DiskSizeEditView::new_emptyable().content_maybe(options.min_lvm_free),
);
- Self { view }
+ Self {
+ view,
+ disk: disk.clone(),
+ }
}
- fn get_values(&mut self) -> Option<LvmBootdiskOptions> {
+ fn get_values(&mut self) -> Option<(Disk, LvmBootdiskOptions)> {
let is_pve = crate::setup_info().config.product == ProxmoxProduct::PVE;
let min_lvm_free_id = if is_pve { 4 } else { 2 };
let max_root_size = if is_pve {
@@ -285,13 +314,16 @@ impl LvmBootdiskOptionsView {
} else {
None
};
- Some(LvmBootdiskOptions {
- total_size: self.view.get_value::<DiskSizeEditView, _>(0)?,
- swap_size: self.view.get_value::<DiskSizeEditView, _>(1),
- max_root_size,
- max_data_size,
- min_lvm_free: self.view.get_value::<DiskSizeEditView, _>(min_lvm_free_id),
- })
+ Some((
+ self.disk.clone(),
+ LvmBootdiskOptions {
+ total_size: self.view.get_value::<DiskSizeEditView, _>(0)?,
+ swap_size: self.view.get_value::<DiskSizeEditView, _>(1),
+ max_root_size,
+ max_data_size,
+ min_lvm_free: self.view.get_value::<DiskSizeEditView, _>(min_lvm_free_id),
+ },
+ ))
}
}
@@ -561,11 +593,10 @@ impl ViewWrapper for ZfsBootdiskOptionsView {
fn advanced_options_view(avail_disks: &[Disk], options_ref: BootdiskOptionsRef) -> impl View {
Dialog::around(AdvancedBootdiskOptionsView::new(
avail_disks,
- &(*options_ref).borrow(),
+ options_ref.clone(),
))
.title("Advanced bootdisk options")
.button("Ok", {
- let options_ref = options_ref.clone();
move |siv| {
let runinfo = siv
.user_data::<InstallerState>()
@@ -728,6 +759,34 @@ fn check_btrfs_raid_config(level: BtrfsRaidLevel, disks: &[Disk]) -> Result<(),
Ok(())
}
+/// Creates a select view for all disks specified.
+///
+/// # Arguments
+///
+/// * `avail_disks` - Disks that should be shown in the select view
+/// * `options_ref` - A `BootdiskOptionsRef`, which is used to set the disk (and its defaults)
+/// when a disk is selected
+/// * `selected_disk` - Optional, specifies which disk should be pre-selected
+fn target_bootdisk_selectview(
+ avail_disks: &[Disk],
+ options_ref: BootdiskOptionsRef,
+ selected_disk: Option<&Disk>,
+) -> SelectView<Disk> {
+ let selected_disk_pos = selected_disk
+ .and_then(|disk| avail_disks.iter().position(|d| d.index == disk.index))
+ .unwrap_or_default();
+
+ SelectView::new()
+ .popup()
+ .with_all(avail_disks.iter().map(|d| (d.to_string(), d.clone())))
+ .selected(selected_disk_pos)
+ .on_submit(move |_, disk| {
+ options_ref.borrow_mut().disks = vec![disk.clone()];
+ options_ref.borrow_mut().advanced =
+ AdvancedBootdiskOptions::Lvm(LvmBootdiskOptions::defaults_from(disk));
+ })
+}
+
#[cfg(test)]
mod tests {
use std::collections::HashMap;
--
2.41.0
^ permalink raw reply [flat|nested] 4+ messages in thread