[pve-devel] [PATCH installer v3 0/2] fix #4856: tui: bootdisk: use correct defaults in advanced dialog

[pve-devel] [PATCH installer v3 0/2] fix #4856: tui: bootdisk: use correct defaults in advanced dialog

[Christoph Heiss]

Fixes #4856 [0].

Patch #1 is preparatory only, #2 the actual fix.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=4856

v1: https://lists.proxmox.com/pipermail/pve-devel/2023-July/058400.html
v2: https://lists.proxmox.com/pipermail/pve-devel/2023-July/058452.html

Changes v1 -> v2:
  * rebased series on latest master

Changes v2 -> v3:
  * rebased series on latest master
  * dropped patch #2, which was obsoleted by other changes

Christoph Heiss (2):
  tui: bootdisk: refactor Rc<RefCell<..>> type into custom type
  fix #4856: tui: bootdisk: use correct defaults in advanced dialog

 proxmox-tui-installer/src/views/bootdisk.rs | 170 ++++++++++++++------
 1 file changed, 119 insertions(+), 51 deletions(-)

--
2.41.0

[pve-devel] [PATCH installer v3 1/2] tui: bootdisk: refactor Rc<RefCell<..>> type into custom type

[Christoph Heiss]

Will be used/passed around quite a lot of times due to future changes,
so simplify it a bit.

No functional changes.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-tui-installer/src/views/bootdisk.rs | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 5272258..6309d74 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -29,9 +29,13 @@ use proxmox_installer_common::{
 /// https://openzfs.github.io/openzfs-docs/Performance%20and%20Tuning/Module%20Parameters.html#zfs-arc-max
 const ZFS_ARC_MIN_SIZE_MIB: usize = 64; // MiB
 
+/// Convience wrapper when needing to take a (interior-mutable) reference to `BootdiskOptions`.
+/// Interior mutability is safe for this case, as it is completely single-threaded.
+pub type BootdiskOptionsRef = Rc<RefCell<BootdiskOptions>>;
+
 pub struct BootdiskOptionsView {
     view: LinearLayout,
-    advanced_options: Rc<RefCell<BootdiskOptions>>,
+    advanced_options: BootdiskOptionsRef,
     boot_type: BootType,
 }
 
@@ -616,17 +620,17 @@ impl ViewWrapper for ZfsBootdiskOptionsView {
 
 fn advanced_options_view(
     runinfo: &RuntimeInfo,
-    options: Rc<RefCell<BootdiskOptions>>,
+    options_ref: BootdiskOptionsRef,
     product_conf: ProductConfig,
 ) -> impl View {
     Dialog::around(AdvancedBootdiskOptionsView::new(
         runinfo,
-        &(*options).borrow(),
+        &(*options_ref).borrow(),
         product_conf,
     ))
     .title("Advanced bootdisk options")
     .button("Ok", {
-        let options_ref = options.clone();
+        let options_ref = options_ref.clone();
         move |siv| {
             let options = siv
                 .call_on_name("advanced-bootdisk-options-dialog", |view: &mut Dialog| {
-- 
2.42.0

[pve-devel] [PATCH installer v3 2/2] fix #4856: tui: bootdisk: use correct defaults in advanced dialog

[Christoph Heiss]

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.

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>
---
 proxmox-tui-installer/src/views/bootdisk.rs | 162 ++++++++++++++------
 1 file changed, 113 insertions(+), 49 deletions(-)

diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 6309d74..4bd504b 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -41,12 +41,16 @@ pub struct BootdiskOptionsView {

 impl BootdiskOptionsView {
     pub fn new(siv: &mut Cursive, runinfo: &RuntimeInfo, 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(runinfo.disks.iter().map(|d| (d.to_string(), d.clone()))),
+                target_bootdisk_selectview(
+                    &runinfo.disks,
+                    advanced_options.clone(),
+                    options.disks.first(),
+                ),
             )
             .with_name("bootdisk-options-target-disk");

@@ -55,8 +59,6 @@ impl BootdiskOptionsView {
             .map(|state| state.setup_info.config.clone())
             .unwrap(); // Safety: InstallerState must always be set

-        let advanced_options = Rc::new(RefCell::new(options.clone()));
-
         let advanced_button = LinearLayout::horizontal()
             .child(DummyView.full_width())
             .child(Button::new("Advanced options", {
@@ -89,20 +91,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];
-        }
-
+        // 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.
+        let options = (*self.advanced_options).clone().into_inner();
         check_disks_4kn_legacy_boot(self.boot_type, &options.disks)?;
         Ok(options)
     }
@@ -117,9 +109,14 @@ struct AdvancedBootdiskOptionsView {
 }

 impl AdvancedBootdiskOptionsView {
-    fn new(runinfo: &RuntimeInfo, options: &BootdiskOptions, product_conf: ProductConfig) -> Self {
+    fn new(
+        runinfo: &RuntimeInfo,
+        options_ref: BootdiskOptionsRef,
+        product_conf: ProductConfig,
+    ) -> Self {
         let filter_btrfs =
             |fstype: &&FsType| -> bool { product_conf.enable_btrfs || !fstype.is_btrfs() };
+        let options = (*options_ref).borrow();

         let fstype_select = SelectView::new()
             .popup()
@@ -136,17 +133,25 @@ impl AdvancedBootdiskOptionsView {
                     .position(|t| *t == options.fstype)
                     .unwrap_or_default(),
             )
-            .on_submit(Self::fstype_on_submit);
+            .on_submit({
+                let options_ref = options_ref.clone();
+                move |siv, fstype| {
+                    Self::fstype_on_submit(siv, fstype, options_ref.clone());
+                }
+            });

         let mut view = LinearLayout::vertical()
             .child(DummyView.full_width())
             .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, &product_conf))
-            }
+            AdvancedBootdiskOptions::Lvm(lvm) => view.add_child(LvmBootdiskOptionsView::new(
+                &options.disks[0],
+                lvm,
+                &product_conf,
+            )),
             AdvancedBootdiskOptions::Zfs(zfs) => {
                 view.add_child(ZfsBootdiskOptionsView::new(runinfo, zfs, &product_conf))
             }
@@ -158,20 +163,44 @@ impl AdvancedBootdiskOptionsView {
         Self { view }
     }

-    fn fstype_on_submit(siv: &mut Cursive, 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.
+    ///
+    /// # Arguments
+    /// * `siv` - Cursive instance
+    /// * `fstype` - The chosen filesystem type by the user, for which the UI should be
+    ///              updated accordingly
+    /// * `options_ref` - [`BootdiskOptionsRef`] where advanced disk options should be saved to
+    fn fstype_on_submit(siv: &mut Cursive, fstype: &FsType, options_ref: BootdiskOptionsRef) {
         let state = siv.user_data::<InstallerState>().unwrap();
         let runinfo = state.runtime_info.clone();
         let product_conf = state.setup_info.config.clone();

+        // 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_with_defaults(&runinfo.disks[0], &product_conf),
-                    ),
+                    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_with_defaults(
+                            &selected_disk,
+                            &product_conf,
+                        ));
+                    }
                     FsType::Zfs(_) => view.add_child(ZfsBootdiskOptionsView::new_with_defaults(
                         &runinfo,
                         &product_conf,
@@ -183,15 +212,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(runinfo.disks.iter().map(|d| (d.to_string(), d.clone()))),
+                        target_bootdisk_selectview(
+                            &runinfo.disks,
+                            options_ref,
+                            selected_lvm_disk.as_ref(),
+                        ),
                     );
                 }
                 other => view.replace_child(0, TextView::new(other.to_string())),
@@ -213,15 +248,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
@@ -263,14 +297,14 @@ impl ViewWrapper for AdvancedBootdiskOptionsView {

 struct LvmBootdiskOptionsView {
     view: FormView,
+    disk: Disk,
     has_extra_fields: bool,
 }

 impl LvmBootdiskOptionsView {
-    fn new(options: &LvmBootdiskOptions, product_conf: &ProductConfig) -> Self {
+    fn new(disk: &Disk, options: &LvmBootdiskOptions, product_conf: &ProductConfig) -> Self {
         let show_extra_fields = product_conf.product == ProxmoxProduct::PVE;

-        // TODO: Set maximum accordingly to disk size
         let view = FormView::new()
             .child(
                 "Total size",
@@ -299,15 +333,16 @@ impl LvmBootdiskOptionsView {

         Self {
             view,
+            disk: disk.clone(),
             has_extra_fields: show_extra_fields,
         }
     }

     fn new_with_defaults(disk: &Disk, product_conf: &ProductConfig) -> Self {
-        Self::new(&LvmBootdiskOptions::defaults_from(disk), product_conf)
+        Self::new(disk, &LvmBootdiskOptions::defaults_from(disk), product_conf)
     }

-    fn get_values(&mut self) -> Option<LvmBootdiskOptions> {
+    fn get_values(&mut self) -> Option<(Disk, LvmBootdiskOptions)> {
         let min_lvm_free_id = if self.has_extra_fields { 4 } else { 2 };

         let max_root_size = self
@@ -319,13 +354,16 @@ impl LvmBootdiskOptionsView {
             .then(|| self.view.get_value::<DiskSizeEditView, _>(3))
             .flatten();

-        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),
+            },
+        ))
     }
 }

@@ -625,12 +663,11 @@ fn advanced_options_view(
 ) -> impl View {
     Dialog::around(AdvancedBootdiskOptionsView::new(
         runinfo,
-        &(*options_ref).borrow(),
+        options_ref.clone(),
         product_conf,
     ))
     .title("Advanced bootdisk options")
     .button("Ok", {
-        let options_ref = options_ref.clone();
         move |siv| {
             let options = siv
                 .call_on_name("advanced-bootdisk-options-dialog", |view: &mut Dialog| {
@@ -666,3 +703,30 @@ fn advanced_options_view(
     .with_name("advanced-bootdisk-options-dialog")
     .max_size((120, 40))
 }
+
+/// Creates a select view for all disks specified.
+///
+/// # Arguments
+///
+/// * `avail_disks` - Disks that should be shown in the select view
+/// * `options_ref` - [`BootdiskOptionsRef`] where advanced disk options should be saved to
+/// * `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));
+        })
+}
--
2.42.0

[pve-devel] applied: [PATCH installer v3 0/2] fix #4856: tui: bootdisk: use correct defaults in advanced dialog

[Thomas Lamprecht]

Am 09/11/2023 um 10:40 schrieb Christoph Heiss:
> Fixes #4856 [0].
> 
> Patch #1 is preparatory only, #2 the actual fix.
> 
> [0] https://bugzilla.proxmox.com/show_bug.cgi?id=4856
> 
> v1: https://lists.proxmox.com/pipermail/pve-devel/2023-July/058400.html
> v2: https://lists.proxmox.com/pipermail/pve-devel/2023-July/058452.html
> 
> Changes v1 -> v2:
>   * rebased series on latest master
> 
> Changes v2 -> v3:
>   * rebased series on latest master
>   * dropped patch #2, which was obsoleted by other changes
> 
> Christoph Heiss (2):
>   tui: bootdisk: refactor Rc<RefCell<..>> type into custom type
>   fix #4856: tui: bootdisk: use correct defaults in advanced dialog
> 

applied series, thanks!