[pve-devel] [PATCH installer 0/6] some small, assorted fixes & cleanups

[pve-devel] [PATCH installer 0/6] some small, assorted fixes & cleanups

[Christoph Heiss]

#1-#4 are pretty self-explanatory, #5 and #6 might be worth bit more
attention.

Christoph Heiss (6):
  tui: drop some leftover, commented-out code
  tui: password: include minimum password length in error message
  tui: network: select matching NIC for IP configuration
  tui: setup: fix disk size for 4Kn block devices
  sys: block: fix possible use of `undef`-value when detecting disk
    sizes
  tui: setup: handle missing disk block size gracefully

 Proxmox/Sys/Block.pm                        | 11 +++++++----
 proxmox-tui-installer/src/main.rs           | 13 +++++++++----
 proxmox-tui-installer/src/options.rs        |  2 +-
 proxmox-tui-installer/src/setup.rs          |  8 ++++++--
 proxmox-tui-installer/src/views/bootdisk.rs | 16 ++++++++++++----
 5 files changed, 35 insertions(+), 15 deletions(-)

--
2.41.0

[pve-devel] [PATCH installer 1/6] tui: drop some leftover, commented-out code

[Christoph Heiss]

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

diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
index 7bfaf9b..83a4d60 100644
--- a/proxmox-tui-installer/src/main.rs
+++ b/proxmox-tui-installer/src/main.rs
@@ -354,8 +354,6 @@ fn yes_no_dialog(
     siv: &mut Cursive,
     title: &str,
     text: &str,
-    // callback_yes: &'static dyn Fn(&mut Cursive),
-    // callback_no: &'static dyn Fn(&mut Cursive),
     callback_yes: Box<dyn Fn(&mut Cursive)>,
     callback_no: Box<dyn Fn(&mut Cursive)>,
 ) {
-- 
2.41.0

[pve-devel] [PATCH installer 2/6] tui: password: include minimum password length in error message

[Christoph Heiss]

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

diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
index 83a4d60..4782fa2 100644
--- a/proxmox-tui-installer/src/main.rs
+++ b/proxmox-tui-installer/src/main.rs
@@ -514,7 +514,7 @@ fn password_dialog(siv: &mut Cursive) -> InstallerView {
                         .unwrap();
 
                 if root_password.len() < 5 {
-                    Err("password too short")
+                    Err("password too short, must be at least 5 characters long")
                 } else if root_password != confirm_password {
                     Err("passwords do not match")
                 } else if email == "mail@example.invalid" {
-- 
2.41.0

[pve-devel] [PATCH installer 3/6] tui: network: select matching NIC for IP configuration

[Christoph Heiss]

Instead of always just selecting an essentially random NIC (depending on
the enumeration), use the correct one for the rest of the
(DHCP-obtained) IP configuration.

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

diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
index 4782fa2..580cb34 100644
--- a/proxmox-tui-installer/src/main.rs
+++ b/proxmox-tui-installer/src/main.rs
@@ -548,13 +548,20 @@ fn password_dialog(siv: &mut Cursive) -> InstallerView {
 fn network_dialog(siv: &mut Cursive) -> InstallerView {
     let state = siv.user_data::<InstallerState>().unwrap();
     let options = &state.options.network;
+    let ifnames = state.runtime_info.network.interfaces.keys();
 
     let inner = FormView::new()
         .child(
             "Management interface",
             SelectView::new()
                 .popup()
-                .with_all_str(state.runtime_info.network.interfaces.keys()),
+                .with_all_str(ifnames.clone())
+                .selected(
+                    ifnames
+                        .clone()
+                        .position(|ifname| ifname == &options.ifname)
+                        .unwrap_or_default(),
+                ),
         )
         .child(
             "Hostname (FQDN)",
-- 
2.41.0

[pve-devel] [PATCH installer 4/6] tui: setup: fix disk size for 4Kn block devices

[Christoph Heiss]

This can be tested by creating a block device with 4K sectorsize using
the following QEMU args:
  -device virtio-blk,drive=testdrive4k,logical_block_size=4096,physical_block_size=4096
  -drive file=/path/to/4k-testdisk.img,if=none,id=testdrive4k

The 4k-testdisk.img was created with:
  qemu-img create -f qcow2 /path/to/4k-testdisk.img 16G

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-tui-installer/src/setup.rs | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/proxmox-tui-installer/src/setup.rs b/proxmox-tui-installer/src/setup.rs
index e51bb4d..c071b80 100644
--- a/proxmox-tui-installer/src/setup.rs
+++ b/proxmox-tui-installer/src/setup.rs
@@ -283,7 +283,10 @@ where
         .map(
             |(index, device, size_mb, model, logical_bsize, _syspath)| Disk {
                 index: index.to_string(),
-                size: (size_mb * logical_bsize as f64) / 1024. / 1024. / 1024.,
+                // Linux always reports the size of block devices in sectors, where one sector is
+                // defined as being 2^9 = 512 bytes in size.
+                // https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/blk_types.h?h=v6.4#n30
+                size: (size_mb * 512.) / 1024. / 1024. / 1024.,
                 block_size: logical_bsize,
                 path: device,
                 model: (!model.is_empty()).then_some(model),
-- 
2.41.0

[pve-devel] [PATCH installer 5/6] sys: block: fix possible use of `undef`-value when detecting disk sizes

[Christoph Heiss]

`$size` and `$logical_bsize` might get unset if there invalid, but then
are unconditionally converted to an int - which throws an error.
This was reported on the forum by a user [0].

Fix it by changing the check a bit to skip the disk immediately if
detecting either of those two values are invalid or simply not present.
The fix for `$logical_bsize` simply avoids trying to convert to value to
an int if invalid.

tl;dr: Should have no impact at all, in the end. The same
`run-env-info.json` is generated w/ and w/o the patch if all disks are
fine. I then also hacked up the script a bit to actually have an invalid
size, this resulted in the "affected" disk simply missing from the disk
array - the expected behavior in this case.

[0] https://forum.proxmox.com/threads/error-installing-proxmox-8.131921/

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/Sys/Block.pm | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/Proxmox/Sys/Block.pm b/Proxmox/Sys/Block.pm
index 26085e3..f76e0f1 100644
--- a/Proxmox/Sys/Block.pm
+++ b/Proxmox/Sys/Block.pm
@@ -90,10 +90,10 @@ my sub hd_list {
 	}
 
 	my $size = file_read_firstline("$bd/size");
+	next if !$size;
 	chomp $size;
-	$size = undef if !($size && $size =~ m/^\d+$/);
+	next if $size !~ m/^\d+$/;
 	$size = int($size);
-	next if !$size;
 
 	my $model = file_read_firstline("$bd/device/model") || '';
 	$model =~ s/^\s+//;
@@ -104,8 +104,11 @@ my sub hd_list {
 
 	my $logical_bsize = file_read_firstline("$bd/queue/logical_block_size") // '';
 	chomp $logical_bsize;
-	$logical_bsize = undef if !($logical_bsize && $logical_bsize =~ m/^\d+$/);
-	$logical_bsize = int($logical_bsize);
+	if ($logical_bsize && $logical_bsize !~ m/^\d+$/) {
+	    $logical_bsize = int($logical_bsize);
+	} else {
+	    $logical_bsize = undef;
+	}
 
 	push @$res, [$count++, $dev_path, $size, $model, $logical_bsize, "/sys/block/$name"];
     }
-- 
2.41.0

Re: [pve-devel] [PATCH installer 5/6] sys: block: fix possible use of `undef`-value when detecting disk sizes

[Wolfgang Bumiller]

On Wed, Aug 09, 2023 at 03:44:24PM +0200, Christoph Heiss wrote:
> `$size` and `$logical_bsize` might get unset if there invalid, but then
> are unconditionally converted to an int - which throws an error.
> This was reported on the forum by a user [0].
> 
> Fix it by changing the check a bit to skip the disk immediately if
> detecting either of those two values are invalid or simply not present.
> The fix for `$logical_bsize` simply avoids trying to convert to value to
> an int if invalid.
> 
> tl;dr: Should have no impact at all, in the end. The same
> `run-env-info.json` is generated w/ and w/o the patch if all disks are
> fine. I then also hacked up the script a bit to actually have an invalid
> size, this resulted in the "affected" disk simply missing from the disk
> array - the expected behavior in this case.
> 
> [0] https://forum.proxmox.com/threads/error-installing-proxmox-8.131921/
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  Proxmox/Sys/Block.pm | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/Proxmox/Sys/Block.pm b/Proxmox/Sys/Block.pm
> index 26085e3..f76e0f1 100644
> --- a/Proxmox/Sys/Block.pm
> +++ b/Proxmox/Sys/Block.pm
> @@ -90,10 +90,10 @@ my sub hd_list {
>  	}
>  
>  	my $size = file_read_firstline("$bd/size");
> +	next if !$size;
>  	chomp $size;
> -	$size = undef if !($size && $size =~ m/^\d+$/);
> +	next if $size !~ m/^\d+$/;
>  	$size = int($size);
> -	next if !$size;

^ not sure it makes sense to remove this, but OTOH, file_read_firstline
already chomps, so the first one already catches at least the zero-sized
device case (which btw. is easily reproducible by simply plugging in a
card reader without a card inserted ;-) )

Re: [pve-devel] [PATCH installer 5/6] sys: block: fix possible use of `undef`-value when detecting disk sizes

[Christoph Heiss]

On Wed, Aug 23, 2023 at 10:31:29AM +0200, Wolfgang Bumiller wrote:
>
> On Wed, Aug 09, 2023 at 03:44:24PM +0200, Christoph Heiss wrote:
> > [..]
> > diff --git a/Proxmox/Sys/Block.pm b/Proxmox/Sys/Block.pm
> > index 26085e3..f76e0f1 100644
> > --- a/Proxmox/Sys/Block.pm
> > +++ b/Proxmox/Sys/Block.pm
> > @@ -90,10 +90,10 @@ my sub hd_list {
> >  	}
> >
> >  	my $size = file_read_firstline("$bd/size");
> > +	next if !$size;
> >  	chomp $size;
> > -	$size = undef if !($size && $size =~ m/^\d+$/);
> > +	next if $size !~ m/^\d+$/;
> >  	$size = int($size);
> > -	next if !$size;
>
> ^ not sure it makes sense to remove this, but OTOH, file_read_firstline
> already chomps, so the first one already catches at least the zero-sized
> device case
Yeah, that was my thought here as well. It's chomped and the regex
should take care of empty values / anything that is not some integer in
any case.

> (which btw. is easily reproducible by simply plugging in a card reader
> without a card inserted ;-) )
I was wondering how such a case could even happen - makes sense tho, TIL
:^)

[pve-devel] [PATCH installer 6/6] tui: setup: handle missing disk block size gracefully

[Christoph Heiss]

As that value can indeed be undefined, handle that (edge-)case
gracefully. There is only one place where it is checked, in the ZFS RAID
setup dialog. Aligns it with how the low-level installer handles that
case too.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-tui-installer/src/options.rs        |  2 +-
 proxmox-tui-installer/src/setup.rs          |  3 ++-
 proxmox-tui-installer/src/views/bootdisk.rs | 16 ++++++++++++----
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/proxmox-tui-installer/src/options.rs b/proxmox-tui-installer/src/options.rs
index dab1730..f18c813 100644
--- a/proxmox-tui-installer/src/options.rs
+++ b/proxmox-tui-installer/src/options.rs
@@ -222,7 +222,7 @@ pub struct Disk {
     pub path: String,
     pub model: Option<String>,
     pub size: f64,
-    pub block_size: usize,
+    pub block_size: Option<usize>,
 }
 
 impl fmt::Display for Disk {
diff --git a/proxmox-tui-installer/src/setup.rs b/proxmox-tui-installer/src/setup.rs
index c071b80..dec91cb 100644
--- a/proxmox-tui-installer/src/setup.rs
+++ b/proxmox-tui-installer/src/setup.rs
@@ -277,7 +277,8 @@ fn deserialize_disks_map<'de, D>(deserializer: D) -> Result<Vec<Disk>, D::Error>
 where
     D: Deserializer<'de>,
 {
-    let disks = <Vec<(usize, String, f64, String, usize, String)>>::deserialize(deserializer)?;
+    let disks =
+        <Vec<(usize, String, f64, String, Option<usize>, String)>>::deserialize(deserializer)?;
     Ok(disks
         .into_iter()
         .map(
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index a018e71..d01495e 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -642,7 +642,9 @@ fn check_zfs_raid_config(
     // See also Proxmox/Install.pm:get_zfs_raid_setup()
 
     for disk in disks {
-        if runinfo.boot_type != BootType::Efi && disk.block_size == 4096 {
+        if runinfo.boot_type != BootType::Efi
+            && disk.block_size.map(|v| v == 4096).unwrap_or_default()
+        {
             return Err("Booting from 4Kn drive in legacy BIOS mode is not supported.".to_owned());
         }
     }
@@ -728,7 +730,7 @@ mod tests {
             path: format!("/dev/dummy{index}"),
             model: Some("Dummy disk".to_owned()),
             size: 1024. * 1024. * 1024. * 8.,
-            block_size: 512,
+            block_size: Some(512),
         }
     }
 
@@ -799,14 +801,20 @@ mod tests {
 
     #[test]
     fn zfs_raid_bios() {
-        let disks = dummy_disks(10);
         let runinfo = dummy_runinfo(BootType::Bios);
 
+        let mut disks = dummy_disks(10);
+        zfs_common_tests(&disks, &runinfo);
+
+        for disk in &mut disks {
+            disk.block_size = None;
+        }
+        // Should behave the same as if an explicit block size of 512 was set
         zfs_common_tests(&disks, &runinfo);
 
         for i in 0..10 {
             let mut disks = dummy_disks(10);
-            disks[i].block_size = 4096;
+            disks[i].block_size = Some(4096);
 
             // Must fail if /any/ of the disks are 4Kn
             assert!(check_zfs_raid_config(&runinfo, ZfsRaidLevel::Raid0, &disks).is_err());
-- 
2.41.0

[pve-devel] applied-series: [PATCH installer 0/6] some small, assorted fixes & cleanups

[Wolfgang Bumiller]

applied series