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

[pve-devel] [PATCH installer v2 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.

v1: https://lists.proxmox.com/pipermail/pve-devel/2023-October/059335.html

Changes v1 -> v2:
  * Rebased on latest master, no actual changes otherwise

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        | 75 ++++++++++-----------
 proxmox-tui-installer/src/setup.rs          | 10 ++-
 proxmox-tui-installer/src/views/bootdisk.rs | 74 +++++++++++++-------
 4 files changed, 95 insertions(+), 98 deletions(-)

--
2.41.0

[pve-devel] [PATCH installer v2 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.

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

diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
index 0a61e39..a342a08 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 abc8c7b..85b39b8 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;
 
@@ -339,49 +339,25 @@ pub struct NetworkOptions {
 
 impl NetworkOptions {
     const DEFAULT_DOMAIN: &str = "example.invalid";
-}
 
-impl Default for NetworkOptions {
-    fn default() -> Self {
-        let fqdn = format!(
-            "{}.{}",
-            crate::current_product().default_hostname(),
-            Self::DEFAULT_DOMAIN
-        );
-        // TODO: Retrieve automatically
-        Self {
+    pub fn defaults_from(setup: &SetupInfo, network: &NetworkInfo) -> Self {
+        let mut this = Self {
             ifname: String::new(),
-            fqdn: fqdn.parse().unwrap(),
+            fqdn: Self::construct_fqdn(network, setup.config.product.default_hostname()),
             // 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;
         }
 
-        let hostname = info
-            .hostname
-            .as_deref()
-            .unwrap_or_else(|| crate::current_product().default_hostname());
-        let domain = info.dns.domain.as_deref().unwrap_or(Self::DEFAULT_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()) {
@@ -394,7 +370,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();
@@ -409,6 +385,23 @@ impl From<&NetworkInfo> for NetworkOptions {
 
         this
     }
+
+    fn construct_fqdn(network: &NetworkInfo, default_hostname: &str) -> Fqdn {
+        let hostname = network.hostname.as_deref().unwrap_or(default_hostname);
+
+        let domain = network
+            .dns
+            .domain
+            .as_deref()
+            .unwrap_or(Self::DEFAULT_DOMAIN);
+
+        Fqdn::from(&format!("{hostname}.{domain}")).unwrap_or_else(|_| {
+            // Safety: This will always result in a valid FQDN, as we control & know
+            // the values of default_hostname (one of "pve", "pmg" or "pbs") and
+            // constant-defined DEFAULT_DOMAIN.
+            Fqdn::from(&format!("{}.{}", default_hostname, Self::DEFAULT_DOMAIN)).unwrap()
+        })
+    }
 }
 
 #[derive(Clone, Debug)]
@@ -460,8 +453,8 @@ mod tests {
     };
     use std::{collections::HashMap, path::PathBuf};
 
-    fn fill_setup_info() {
-        crate::init_setup_info(SetupInfo {
+    fn dummy_setup_info() -> SetupInfo {
+        SetupInfo {
             config: ProductConfig {
                 fullname: "Proxmox VE".to_owned(),
                 product: ProxmoxProduct::PVE,
@@ -474,12 +467,12 @@ mod tests {
             locations: IsoLocations {
                 iso: PathBuf::new(),
             },
-        });
+        }
     }
 
     #[test]
     fn network_options_from_setup_network_info() {
-        fill_setup_info();
+        let setup = dummy_setup_info();
 
         let mut interfaces = HashMap::new();
         interfaces.insert(
@@ -512,7 +505,7 @@ mod tests {
         };
 
         assert_eq!(
-            NetworkOptions::from(&info),
+            NetworkOptions::defaults_from(&setup, &info),
             NetworkOptions {
                 ifname: "eth0".to_owned(),
                 fqdn: Fqdn::from("foo.bar.com").unwrap(),
@@ -524,7 +517,7 @@ mod tests {
 
         info.hostname = None;
         assert_eq!(
-            NetworkOptions::from(&info),
+            NetworkOptions::defaults_from(&setup, &info),
             NetworkOptions {
                 ifname: "eth0".to_owned(),
                 fqdn: Fqdn::from("pve.bar.com").unwrap(),
@@ -536,7 +529,7 @@ mod tests {
 
         info.dns.domain = None;
         assert_eq!(
-            NetworkOptions::from(&info),
+            NetworkOptions::defaults_from(&setup, &info),
             NetworkOptions {
                 ifname: "eth0".to_owned(),
                 fqdn: Fqdn::from("pve.example.invalid").unwrap(),
@@ -548,7 +541,7 @@ mod tests {
 
         info.hostname = Some("foo".to_owned());
         assert_eq!(
-            NetworkOptions::from(&info),
+            NetworkOptions::defaults_from(&setup, &info),
             NetworkOptions {
                 ifname: "eth0".to_owned(),
                 fqdn: Fqdn::from("foo.example.invalid").unwrap(),
diff --git a/proxmox-tui-installer/src/setup.rs b/proxmox-tui-installer/src/setup.rs
index 7238131..5575759 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 v2 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.

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 v2 3/3] tui: remove obsolete, global `SetupInfo` state

[Christoph Heiss]

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 a342a08..81fe3ca 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

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

[Thomas Lamprecht]

Am 25/10/2023 um 10:56 schrieb 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.
> 
> v1: https://lists.proxmox.com/pipermail/pve-devel/2023-October/059335.html
> 
> Changes v1 -> v2:
>   * Rebased on latest master, no actual changes otherwise
> 
> 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

applied series, thanks!