[pve-devel] [PATCH installer 0/3] tui: remove global, unsafe setup info

[pve-devel] [PATCH installer 0/3] tui: remove global, unsafe setup info

[Christoph Heiss]

Removes the `static mut` for holding a `SetupInfo` instance.

This is done by either passing the needed info as parameter, or in some
cases, the needed information is already available through other means.

Not only does it get rid of some ugly, unsafe code, it is needed anyway
as a prerequisite by Aaron for pulling out non-TUI-related code into a
separate, shared crate.

No functional changes overall.

Christoph Heiss (3):
  tui: refactor `NetworkOptions` to have a `defaults_from()` function
  tui: bootdisk: pass down product info to advanced dialog
  tui: remove obsolete, global `SetupInfo` state

 proxmox-tui-installer/src/main.rs           | 34 +++-------
 proxmox-tui-installer/src/options.rs        | 36 ++++------
 proxmox-tui-installer/src/setup.rs          | 10 ++-
 proxmox-tui-installer/src/views/bootdisk.rs | 74 ++++++++++++++-------
 4 files changed, 74 insertions(+), 80 deletions(-)

--
2.41.0

[pve-devel] [PATCH installer 1/3] tui: refactor `NetworkOptions` to have a `defaults_from()` function

[Christoph Heiss]

This aligns it with the other constructors for options struct by
introducing a `::defaults_from()` function.

No functional changes.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-tui-installer/src/main.rs    | 15 ++++++------
 proxmox-tui-installer/src/options.rs | 36 ++++++++++------------------
 proxmox-tui-installer/src/setup.rs   | 10 ++++----
 3 files changed, 24 insertions(+), 37 deletions(-)

diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
index 3f01713..ea8c8d9 100644
--- a/proxmox-tui-installer/src/main.rs
+++ b/proxmox-tui-installer/src/main.rs
@@ -166,7 +166,6 @@ enum InstallerStep {
 #[derive(Clone)]
 struct InstallerState {
     options: InstallerOptions,
-    /// FIXME: Remove:
     setup_info: SetupInfo,
     runtime_info: RuntimeInfo,
     locales: LocaleInfo,
@@ -184,7 +183,7 @@ fn main() {
         _ => cfg!(debug_assertions),
     };

-    let (locales, runtime_info) = match installer_setup(in_test_mode) {
+    let (setup_info, locales, runtime_info) = match installer_setup(in_test_mode) {
         Ok(result) => result,
         Err(err) => initial_setup_error(&mut siv, &err),
     };
@@ -197,10 +196,10 @@ fn main() {
             bootdisk: BootdiskOptions::defaults_from(&runtime_info.disks[0]),
             timezone: TimezoneOptions::defaults_from(&runtime_info, &locales),
             password: Default::default(),
-            network: NetworkOptions::from(&runtime_info.network),
+            network: NetworkOptions::defaults_from(&setup_info, &runtime_info.network),
             autoreboot: false,
         },
-        setup_info: setup_info().clone(), // FIXME: REMOVE
+        setup_info,
         runtime_info,
         locales,
         steps: HashMap::new(),
@@ -211,20 +210,20 @@ fn main() {
     siv.run();
 }

-fn installer_setup(in_test_mode: bool) -> Result<(LocaleInfo, RuntimeInfo), String> {
+fn installer_setup(in_test_mode: bool) -> Result<(SetupInfo, LocaleInfo, RuntimeInfo), String> {
     let base_path = if in_test_mode { "./testdir" } else { "/" };
     let mut path = PathBuf::from(base_path);

     path.push("run");
     path.push("proxmox-installer");

-    let installer_info = {
+    let installer_info: SetupInfo = {
         let mut path = path.clone();
         path.push("iso-info.json");

         setup::read_json(&path).map_err(|err| format!("Failed to retrieve setup info: {err}"))?
     };
-    init_setup_info(installer_info);
+    init_setup_info(installer_info.clone());

     let locale_info = {
         let mut path = path.clone();
@@ -245,7 +244,7 @@ fn installer_setup(in_test_mode: bool) -> Result<(LocaleInfo, RuntimeInfo), Stri
     if runtime_info.disks.is_empty() {
         Err("The installer could not find any supported hard disks.".to_owned())
     } else {
-        Ok((locale_info, runtime_info))
+        Ok((installer_info, locale_info, runtime_info))
     }
 }

diff --git a/proxmox-tui-installer/src/options.rs b/proxmox-tui-installer/src/options.rs
index a0a81c9..56ad7f2 100644
--- a/proxmox-tui-installer/src/options.rs
+++ b/proxmox-tui-installer/src/options.rs
@@ -1,7 +1,7 @@
 use std::net::{IpAddr, Ipv4Addr};
 use std::{cmp, fmt};

-use crate::setup::{LocaleInfo, NetworkInfo, RuntimeInfo};
+use crate::setup::{LocaleInfo, NetworkInfo, RuntimeInfo, SetupInfo};
 use crate::utils::{CidrAddress, Fqdn};
 use crate::SummaryOption;

@@ -337,43 +337,33 @@ pub struct NetworkOptions {
     pub dns_server: IpAddr,
 }

-impl Default for NetworkOptions {
-    fn default() -> Self {
-        let fqdn = format!(
-            "{}.example.invalid",
-            crate::current_product().default_hostname()
-        );
-        // TODO: Retrieve automatically
-        Self {
+impl NetworkOptions {
+    pub fn defaults_from(setup: &SetupInfo, network: &NetworkInfo) -> Self {
+        let hostname = setup.config.product.default_hostname();
+
+        let mut this = Self {
             ifname: String::new(),
-            fqdn: fqdn.parse().unwrap(),
+            fqdn: Fqdn::from(&format!("{hostname}.example.invalid")).unwrap(),
             // Safety: The provided mask will always be valid.
             address: CidrAddress::new(Ipv4Addr::UNSPECIFIED, 0).unwrap(),
             gateway: Ipv4Addr::UNSPECIFIED.into(),
             dns_server: Ipv4Addr::UNSPECIFIED.into(),
-        }
-    }
-}
-
-impl From<&NetworkInfo> for NetworkOptions {
-    fn from(info: &NetworkInfo) -> Self {
-        let mut this = Self::default();
+        };

-        if let Some(ip) = info.dns.dns.first() {
+        if let Some(ip) = network.dns.dns.first() {
             this.dns_server = *ip;
         }

-        if let Some(domain) = &info.dns.domain {
-            let hostname = crate::current_product().default_hostname();
+        if let Some(domain) = &network.dns.domain {
             if let Ok(fqdn) = Fqdn::from(&format!("{hostname}.{domain}")) {
                 this.fqdn = fqdn;
             }
         }

-        if let Some(routes) = &info.routes {
+        if let Some(routes) = &network.routes {
             let mut filled = false;
             if let Some(gw) = &routes.gateway4 {
-                if let Some(iface) = info.interfaces.get(&gw.dev) {
+                if let Some(iface) = network.interfaces.get(&gw.dev) {
                     this.ifname = iface.name.clone();
                     if let Some(addresses) = &iface.addresses {
                         if let Some(addr) = addresses.iter().find(|addr| addr.is_ipv4()) {
@@ -386,7 +376,7 @@ impl From<&NetworkInfo> for NetworkOptions {
             }
             if !filled {
                 if let Some(gw) = &routes.gateway6 {
-                    if let Some(iface) = info.interfaces.get(&gw.dev) {
+                    if let Some(iface) = network.interfaces.get(&gw.dev) {
                         if let Some(addresses) = &iface.addresses {
                             if let Some(addr) = addresses.iter().find(|addr| addr.is_ipv6()) {
                                 this.ifname = iface.name.clone();
diff --git a/proxmox-tui-installer/src/setup.rs b/proxmox-tui-installer/src/setup.rs
index 942e319..493bfaf 100644
--- a/proxmox-tui-installer/src/setup.rs
+++ b/proxmox-tui-installer/src/setup.rs
@@ -196,12 +196,10 @@ impl From<InstallerOptions> for InstallConfig {

             mngmt_nic: options.network.ifname,

-            hostname: options
-                .network
-                .fqdn
-                .host()
-                .unwrap_or_else(|| crate::current_product().default_hostname())
-                .to_owned(),
+            // Safety: At this point, it is know that we have a valid FQDN, as
+            // this is set by the TUI network panel, which only lets the user
+            // continue if a valid FQDN is provided.
+            hostname: options.network.fqdn.host().expect("valid FQDN").to_owned(),
             domain: options.network.fqdn.domain(),
             cidr: options.network.address,
             gateway: options.network.gateway,
--
2.42.0

[pve-devel] [PATCH installer 2/3] tui: bootdisk: pass down product info to advanced dialog

[Christoph Heiss]

Enables the advanced and LVM dialog to determine what options (max
root/data size and Btrfs RAIDs) by itself, without needing to resort to
the global `setup_info()` function.

No functional changes.

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

diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index dbd13ea..ba08c8b 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -16,7 +16,7 @@ use crate::{
         FsType, LvmBootdiskOptions, ZfsBootdiskOptions, ZfsRaidLevel, FS_TYPES,
         ZFS_CHECKSUM_OPTIONS, ZFS_COMPRESS_OPTIONS,
     },
-    setup::BootType,
+    setup::{BootType, ProductConfig},
 };
 use crate::{setup::ProxmoxProduct, InstallerState};

@@ -37,6 +37,11 @@ impl BootdiskOptionsView {
             )
             .with_name("bootdisk-options-target-disk");

+        let product_conf = siv
+            .user_data::<InstallerState>()
+            .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()
@@ -45,7 +50,11 @@ impl BootdiskOptionsView {
                 let disks = 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(
+                        &disks,
+                        options.clone(),
+                        product_conf.clone(),
+                    ));
                 }
             }));

@@ -95,10 +104,9 @@ struct AdvancedBootdiskOptionsView {
 }

 impl AdvancedBootdiskOptionsView {
-    fn new(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() };
+    fn new(disks: &[Disk], options: &BootdiskOptions, product_conf: ProductConfig) -> Self {
+        let filter_btrfs =
+            |fstype: &&FsType| -> bool { product_conf.enable_btrfs || !fstype.is_btrfs() };

         let fstype_select = SelectView::new()
             .popup()
@@ -126,7 +134,10 @@ impl AdvancedBootdiskOptionsView {
             .child(DummyView.full_width());

         match &options.advanced {
-            AdvancedBootdiskOptions::Lvm(lvm) => view.add_child(LvmBootdiskOptionsView::new(lvm)),
+            AdvancedBootdiskOptions::Lvm(lvm) => view.add_child(LvmBootdiskOptionsView::new(
+                lvm,
+                product_conf.product == ProxmoxProduct::PVE,
+            )),
             AdvancedBootdiskOptions::Zfs(zfs) => {
                 view.add_child(ZfsBootdiskOptionsView::new(disks, zfs))
             }
@@ -139,6 +150,11 @@ impl AdvancedBootdiskOptionsView {
     }

     fn fstype_on_submit(siv: &mut Cursive, disks: &[Disk], fstype: &FsType) {
+        let is_pve = siv
+            .user_data::<InstallerState>()
+            .map(|state| state.setup_info.config.product == ProxmoxProduct::PVE)
+            .unwrap_or_default();
+
         siv.call_on_name("advanced-bootdisk-options-dialog", |view: &mut Dialog| {
             if let Some(AdvancedBootdiskOptionsView { view }) =
                 view.get_content_mut().downcast_mut()
@@ -147,6 +163,7 @@ impl AdvancedBootdiskOptionsView {
                 match fstype {
                     FsType::Ext4 | FsType::Xfs => view.add_child(LvmBootdiskOptionsView::new(
                         &LvmBootdiskOptions::defaults_from(&disks[0]),
+                        is_pve,
                     )),
                     FsType::Zfs(_) => view.add_child(ZfsBootdiskOptionsView::new(
                         disks,
@@ -240,11 +257,11 @@ impl ViewWrapper for AdvancedBootdiskOptionsView {

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

 impl LvmBootdiskOptionsView {
-    fn new(options: &LvmBootdiskOptions) -> Self {
-        let is_pve = crate::setup_info().config.product == ProxmoxProduct::PVE;
+    fn new(options: &LvmBootdiskOptions, show_extra_fields: bool) -> Self {
         // TODO: Set maximum accordingly to disk size
         let view = FormView::new()
             .child(
@@ -258,12 +275,12 @@ impl LvmBootdiskOptionsView {
                 DiskSizeEditView::new_emptyable().content_maybe(options.swap_size),
             )
             .child_conditional(
-                is_pve,
+                show_extra_fields,
                 "Maximum root volume size",
                 DiskSizeEditView::new_emptyable().content_maybe(options.max_root_size),
             )
             .child_conditional(
-                is_pve,
+                show_extra_fields,
                 "Maximum data volume size",
                 DiskSizeEditView::new_emptyable().content_maybe(options.max_data_size),
             )
@@ -272,22 +289,24 @@ impl LvmBootdiskOptionsView {
                 DiskSizeEditView::new_emptyable().content_maybe(options.min_lvm_free),
             );

-        Self { view }
+        Self {
+            view,
+            has_extra_fields: show_extra_fields,
+        }
     }

     fn get_values(&mut self) -> Option<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 {
-            self.view.get_value::<DiskSizeEditView, _>(2)
-        } else {
-            None
-        };
-        let max_data_size = if is_pve {
-            self.view.get_value::<DiskSizeEditView, _>(3)
-        } else {
-            None
-        };
+        let min_lvm_free_id = if self.has_extra_fields { 4 } else { 2 };
+
+        let max_root_size = self
+            .has_extra_fields
+            .then(|| self.view.get_value::<DiskSizeEditView, _>(2))
+            .flatten();
+        let max_data_size = self
+            .has_extra_fields
+            .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),
@@ -552,10 +571,15 @@ impl ViewWrapper for ZfsBootdiskOptionsView {
     cursive::wrap_impl!(self.view: MultiDiskOptionsView<FormView>);
 }

-fn advanced_options_view(disks: &[Disk], options: Rc<RefCell<BootdiskOptions>>) -> impl View {
+fn advanced_options_view(
+    disks: &[Disk],
+    options: Rc<RefCell<BootdiskOptions>>,
+    product_conf: ProductConfig,
+) -> impl View {
     Dialog::around(AdvancedBootdiskOptionsView::new(
         disks,
         &(*options).borrow(),
+        product_conf,
     ))
     .title("Advanced bootdisk options")
     .button("Ok", {
--
2.42.0

[pve-devel] [PATCH installer 3/3] tui: remove obsolete, global `SetupInfo` state

[Christoph Heiss]

No functional changes.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-tui-installer/src/main.rs | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
index ea8c8d9..49f378b 100644
--- a/proxmox-tui-installer/src/main.rs
+++ b/proxmox-tui-installer/src/main.rs
@@ -1,3 +1,5 @@
+#![forbid(unsafe_code)]
+
 use std::{
     collections::HashMap,
     env,
@@ -50,24 +52,6 @@ const PROXMOX_LOGO: &str = r#"
 |  __/| | | (_) >  <| | | | | | (_) >  <
 |_|   |_|  \___/_/\_\_| |_| |_|\___/_/\_\ "#;

-/// ISO information is available globally.
-static mut SETUP_INFO: Option<SetupInfo> = None;
-
-pub fn setup_info() -> &'static SetupInfo {
-    unsafe { SETUP_INFO.as_ref().unwrap() }
-}
-
-fn init_setup_info(info: SetupInfo) {
-    unsafe {
-        SETUP_INFO = Some(info);
-    }
-}
-
-#[inline]
-pub fn current_product() -> setup::ProxmoxProduct {
-    setup_info().config.product
-}
-
 struct InstallerView {
     view: ResizedView<Dialog>,
 }
@@ -223,7 +207,6 @@ fn installer_setup(in_test_mode: bool) -> Result<(SetupInfo, LocaleInfo, Runtime

         setup::read_json(&path).map_err(|err| format!("Failed to retrieve setup info: {err}"))?
     };
-    init_setup_info(installer_info.clone());

     let locale_info = {
         let mut path = path.clone();
--
2.42.0