Re: [pve-devel] [PATCH proxmox-firewall 1/1] firewall: properly reject ipv6 traffic
v2 available: https://lists.proxmox.com/pipermail/pve-devel/2024-May/063839.html On 5/13/24 13:35, Stefan Hanreich wrote: > ICMPv6 has different message types for rejecting traffic. With ICMP we > used host-prohibited as rejection type, which doesn't exist in ICMPv6. > Add an additional rule for IPv6, so it uses admin-prohibited. > > Signed-off-by: Stefan Hanreich > --- > proxmox-firewall/resources/proxmox-firewall.nft | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/proxmox-firewall/resources/proxmox-firewall.nft > b/proxmox-firewall/resources/proxmox-firewall.nft > index f36bf3b..0a220bf 100644 > --- a/proxmox-firewall/resources/proxmox-firewall.nft > +++ b/proxmox-firewall/resources/proxmox-firewall.nft > @@ -75,8 +75,9 @@ table inet proxmox-firewall { > ip saddr 224.0.0.0/4 drop > > meta l4proto tcp reject with tcp reset > -meta l4proto icmp reject with icmp type port-unreachable > +meta l4proto icmp reject with icmpx type port-unreachable > reject with icmp type host-prohibited > +reject with icmpv6 type admin-prohibited > } > > set v4-dc/management { > @@ -289,8 +290,9 @@ table bridge proxmox-firewall-guests { > ip saddr 224.0.0.0/4 drop > > meta l4proto tcp reject with tcp reset > -meta l4proto icmp reject with icmp type port-unreachable > +meta l4proto icmp reject with icmpx type port-unreachable > reject with icmp type host-prohibited > +reject with icmpv6 type admin-prohibited > } > > chain after-vm-in { ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer 5/6] fix #5250: auto-installer: expose new btrfs `compress` option
Signed-off-by: Christoph Heiss --- proxmox-auto-installer/src/answer.rs | 6 +- proxmox-auto-installer/src/utils.rs | 7 ++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/proxmox-auto-installer/src/answer.rs b/proxmox-auto-installer/src/answer.rs index aab7198..add36a3 100644 --- a/proxmox-auto-installer/src/answer.rs +++ b/proxmox-auto-installer/src/answer.rs @@ -1,6 +1,9 @@ use clap::ValueEnum; use proxmox_installer_common::{ -options::{BtrfsRaidLevel, FsType, ZfsChecksumOption, ZfsCompressOption, ZfsRaidLevel}, +options::{ +BtrfsCompressOption, BtrfsRaidLevel, FsType, ZfsChecksumOption, ZfsCompressOption, +ZfsRaidLevel, +}, utils::{CidrAddress, Fqdn}, }; use serde::{Deserialize, Serialize}; @@ -269,6 +272,7 @@ pub struct LvmOptions { pub struct BtrfsOptions { pub hdsize: Option, pub raid: Option, +pub compress: Option, } #[derive(Clone, Deserialize, Serialize, Debug, PartialEq)] diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs index 30b6196..b08c617 100644 --- a/proxmox-auto-installer/src/utils.rs +++ b/proxmox-auto-installer/src/utils.rs @@ -10,7 +10,9 @@ use crate::{ }; use proxmox_installer_common::{ options::{FsType, NetworkOptions, ZfsChecksumOption, ZfsCompressOption}, -setup::{InstallConfig, InstallZfsOption, LocaleInfo, RuntimeInfo, SetupInfo}, +setup::{ +InstallBtrfsOption, InstallConfig, InstallZfsOption, LocaleInfo, RuntimeInfo, SetupInfo, +}, }; use serde::{Deserialize, Serialize}; @@ -377,6 +379,9 @@ pub fn parse_answer( config.hdsize = btrfs .hdsize .unwrap_or(runtime_info.disks[first_selected_disk].size); +config.btrfs_opts = Some(InstallBtrfsOption { +compress: btrfs.compress.unwrap_or_default(), +}) } } Ok(config) -- 2.44.0 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer 4/6] fix #5250: tui: expose new btrfs `compress` option
Signed-off-by: Christoph Heiss --- proxmox-tui-installer/src/views/bootdisk.rs | 34 +++-- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs index 107fc9c..7a34d63 100644 --- a/proxmox-tui-installer/src/views/bootdisk.rs +++ b/proxmox-tui-installer/src/views/bootdisk.rs @@ -19,8 +19,9 @@ use proxmox_installer_common::{ check_zfs_raid_config, }, options::{ -AdvancedBootdiskOptions, BootdiskOptions, BtrfsBootdiskOptions, BtrfsCompressOption, Disk, -FsType, LvmBootdiskOptions, ZfsBootdiskOptions, ZFS_CHECKSUM_OPTIONS, ZFS_COMPRESS_OPTIONS, +AdvancedBootdiskOptions, BootdiskOptions, BtrfsBootdiskOptions, Disk, FsType, +LvmBootdiskOptions, ZfsBootdiskOptions, BTRFS_COMPRESS_OPTIONS, ZFS_CHECKSUM_OPTIONS, +ZFS_COMPRESS_OPTIONS, }, setup::{BootType, ProductConfig, ProxmoxProduct, RuntimeInfo}, }; @@ -521,12 +522,23 @@ struct BtrfsBootdiskOptionsView { impl BtrfsBootdiskOptionsView { fn new(disks: &[Disk], options: ) -> Self { -let view = MultiDiskOptionsView::new( -disks, -_disks, -FormView::new().child("hdsize", DiskSizeEditView::new().content(options.disk_size)), -) -.top_panel(TextView::new("Btrfs integration is a technology preview!").center()); +let inner = FormView::new() +.child( +"compress", +SelectView::new() +.popup() +.with_all(BTRFS_COMPRESS_OPTIONS.iter().map(|o| (o.to_string(), *o))) +.selected( +BTRFS_COMPRESS_OPTIONS +.iter() +.position(|o| *o == options.compress) +.unwrap_or_default(), +), +) +.child("hdsize", DiskSizeEditView::new().content(options.disk_size)); + +let view = MultiDiskOptionsView::new(disks, _disks, inner) +.top_panel(TextView::new("Btrfs integration is a technology preview!").center()); Self { view } } @@ -537,14 +549,16 @@ impl BtrfsBootdiskOptionsView { fn get_values( self) -> Option<(Vec, BtrfsBootdiskOptions)> { let (disks, selected_disks) = self.view.get_disks_and_selection()?; -let disk_size = self.view.inner_mut()?.get_value::(0)?; +let view = self.view.inner_mut()?; +let disk_size = view.get_value::(1)?; +let compress = view.get_value::, _>(0)?; Some(( disks, BtrfsBootdiskOptions { disk_size, selected_disks, -compress: BtrfsCompressOption::default(), +compress, }, )) } -- 2.44.0 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer 0/6] fix #5250: add btrfs `compress` mount option support
Pretty much as it says on the tin, this adds the `compress` mount option for Btrfs much in the same way as the ZFS equivalent. In regards to the discussion in #5250 - `compress` is used. For the detailed why, see commit #2. Christoph Heiss (6): fix #5250: install: config: add new `btrfs_opts` with `compress` config option fix #5250: install: write btrfs `compress` option to fstab fix #5250: proxinstall: expose new btrfs `compress` option fix #5250: tui: expose new btrfs `compress` option fix #5250: auto-installer: expose new btrfs `compress` option auto-installer: add btrfs answer parsing tests Proxmox/Install.pm| 11 ++- Proxmox/Install/Config.pm | 15 + proxinstall | 15 + proxmox-auto-installer/src/answer.rs | 6 +++- proxmox-auto-installer/src/utils.rs | 8 - .../tests/resources/parse_answer/btrfs.json | 24 ++ .../tests/resources/parse_answer/btrfs.toml | 17 ++ proxmox-installer-common/src/options.rs | 31 +++ proxmox-installer-common/src/setup.rs | 21 +++-- proxmox-tui-installer/src/setup.rs| 2 ++ proxmox-tui-installer/src/views/bootdisk.rs | 31 ++- 11 files changed, 168 insertions(+), 13 deletions(-) create mode 100644 proxmox-auto-installer/tests/resources/parse_answer/btrfs.json create mode 100644 proxmox-auto-installer/tests/resources/parse_answer/btrfs.toml -- 2.44.0 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH proxmox-firewall v2 1/1] firewall: properly reject ipv6 traffic
ICMPv6 has different message types for rejecting traffic. With ICMP we used host-prohibited as rejection type, which doesn't exist in ICMPv6. Add an additional rule for IPv6, so it uses admin-prohibited. Additionally, add a terminal drop statement in order to prevent any traffic that does not get matched from bypassing the reject chain. Signed-off-by: Stefan Hanreich --- Changes from v1 -> v2: * add a terminal drop statement to prevent any unmatched traffic from bypassing the reject chain * properly match ICMPv6 traffic via l4proto proxmox-firewall/resources/proxmox-firewall.nft | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/proxmox-firewall/resources/proxmox-firewall.nft b/proxmox-firewall/resources/proxmox-firewall.nft index f36bf3b..f60f8b5 100644 --- a/proxmox-firewall/resources/proxmox-firewall.nft +++ b/proxmox-firewall/resources/proxmox-firewall.nft @@ -75,8 +75,10 @@ table inet proxmox-firewall { ip saddr 224.0.0.0/4 drop meta l4proto tcp reject with tcp reset -meta l4proto icmp reject with icmp type port-unreachable +meta l4proto { icmp, ipv6-icmp } reject with icmpx type port-unreachable reject with icmp type host-prohibited +reject with icmpv6 type admin-prohibited +drop } set v4-dc/management { @@ -289,8 +291,10 @@ table bridge proxmox-firewall-guests { ip saddr 224.0.0.0/4 drop meta l4proto tcp reject with tcp reset -meta l4proto icmp reject with icmp type port-unreachable +meta l4proto { icmp, ipv6-icmp } reject with icmpx type port-unreachable reject with icmp type host-prohibited +reject with icmpv6 type admin-prohibited +drop } chain after-vm-in { -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer 1/6] fix #5250: install: config: add new `btrfs_opts` with `compress` config option
Signed-off-by: Christoph Heiss --- Proxmox/Install/Config.pm | 15 ++ proxmox-auto-installer/src/utils.rs | 1 + proxmox-installer-common/src/options.rs | 31 + proxmox-installer-common/src/setup.rs | 21 -- proxmox-tui-installer/src/setup.rs | 2 ++ proxmox-tui-installer/src/views/bootdisk.rs | 5 ++-- 6 files changed, 71 insertions(+), 4 deletions(-) diff --git a/Proxmox/Install/Config.pm b/Proxmox/Install/Config.pm index ecd8a74..09edf11 100644 --- a/Proxmox/Install/Config.pm +++ b/Proxmox/Install/Config.pm @@ -79,6 +79,9 @@ my sub init_cfg { copies => 1, arc_max => Proxmox::Install::RunEnv::default_zfs_arc_max(), # in MiB }, + btrfs_opts => { + compress => 'off', + }, # TODO: single disk selection config target_hd => undef, disk_selection => {}, @@ -173,6 +176,18 @@ sub get_zfs_opt { return defined($k) ? $zfs_opts->{$k} : $zfs_opts; } +sub set_btrfs_opt { +my ($k, $v) = @_; +my $opts = get('btrfs_opts'); +croak "unknown btrfs opts key '$k'" if !exists($opts->{$k}); +$opts->{$k} = $v; +} +sub get_btrfs_opt { +my ($k) = @_; +my $opts = get('btrfs_opts'); +return defined($k) ? $opts->{$k} : $opts; +} + sub set_target_hd { set_key('target_hd', $_[0]); } sub get_target_hd { return get('target_hd'); } diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs index 202ad41..30b6196 100644 --- a/proxmox-auto-installer/src/utils.rs +++ b/proxmox-auto-installer/src/utils.rs @@ -326,6 +326,7 @@ pub fn parse_answer( minfree: None, maxvz: None, zfs_opts: None, +btrfs_opts: None, target_hd: None, disk_selection: BTreeMap::new(), lvm_auto_rename: 1, diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs index e77914b..972b66c 100644 --- a/proxmox-installer-common/src/options.rs +++ b/proxmox-installer-common/src/options.rs @@ -102,10 +102,40 @@ impl LvmBootdiskOptions { } } +/// See the accompanying mount option in btrfs(5). +#[derive(Copy, Clone, Debug, Default, Deserialize, Eq, PartialEq)] +#[serde(rename_all(deserialize = "lowercase"))] +pub enum BtrfsCompressOption { +On, +#[default] +Off, +Zlib, +Lzo, +Zstd, +} + +impl fmt::Display for BtrfsCompressOption { +fn fmt(, f: fmt::Formatter) -> fmt::Result { +write!(f, "{}", format!("{self:?}").to_lowercase()) +} +} + +impl From<> for String { +fn from(value: ) -> Self { +value.to_string() +} +} + +pub const BTRFS_COMPRESS_OPTIONS: &[BtrfsCompressOption] = { +use BtrfsCompressOption::*; +&[On, Off, Zlib, Lzo, Zstd] +}; + #[derive(Clone, Debug)] pub struct BtrfsBootdiskOptions { pub disk_size: f64, pub selected_disks: Vec, +pub compress: BtrfsCompressOption, } impl BtrfsBootdiskOptions { @@ -115,6 +145,7 @@ impl BtrfsBootdiskOptions { Self { disk_size: disk.size, selected_disks: (0..disks.len()).collect(), +compress: BtrfsCompressOption::default(), } } } diff --git a/proxmox-installer-common/src/setup.rs b/proxmox-installer-common/src/setup.rs index 64d05af..81f3533 100644 --- a/proxmox-installer-common/src/setup.rs +++ b/proxmox-installer-common/src/setup.rs @@ -13,8 +13,8 @@ use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; use crate::{ options::{ -BtrfsRaidLevel, Disk, FsType, ZfsBootdiskOptions, ZfsChecksumOption, ZfsCompressOption, -ZfsRaidLevel, +BtrfsBootdiskOptions, BtrfsCompressOption, BtrfsRaidLevel, Disk, FsType, +ZfsBootdiskOptions, ZfsChecksumOption, ZfsCompressOption, ZfsRaidLevel, }, utils::CidrAddress, }; @@ -220,6 +220,20 @@ impl From for InstallZfsOption { } } +#[derive(Debug, Deserialize, Serialize)] +pub struct InstallBtrfsOption { +#[serde(serialize_with = "serialize_as_display")] +pub compress: BtrfsCompressOption, +} + +impl From for InstallBtrfsOption { +fn from(opts: BtrfsBootdiskOptions) -> Self { +InstallBtrfsOption { +compress: opts.compress, +} +} +} + pub fn read_json Deserialize<'de>, P: AsRef>(path: P) -> Result { let file = File::open(path).map_err(|err| err.to_string())?; let reader = BufReader::new(file); @@ -466,6 +480,9 @@ pub struct InstallConfig { #[serde(skip_serializing_if = "Option::is_none")] pub zfs_opts: Option, +#[serde(skip_serializing_if = "Option::is_none")] +pub btrfs_opts: Option, + #[serde( serialize_with = "serialize_disk_opt", skip_serializing_if = "Option::is_none", diff --git a/proxmox-tui-installer/src/setup.rs b/proxmox-tui-installer/src/setup.rs index 8c01e42..2622c8e 100644 --- a/proxmox-tui-installer/src/setup.rs +++
[pve-devel] [PATCH installer 6/6] auto-installer: add btrfs answer parsing tests
Signed-off-by: Christoph Heiss --- .../tests/resources/parse_answer/btrfs.json | 24 +++ .../tests/resources/parse_answer/btrfs.toml | 17 + 2 files changed, 41 insertions(+) create mode 100644 proxmox-auto-installer/tests/resources/parse_answer/btrfs.json create mode 100644 proxmox-auto-installer/tests/resources/parse_answer/btrfs.toml diff --git a/proxmox-auto-installer/tests/resources/parse_answer/btrfs.json b/proxmox-auto-installer/tests/resources/parse_answer/btrfs.json new file mode 100644 index 000..d6cc30d --- /dev/null +++ b/proxmox-auto-installer/tests/resources/parse_answer/btrfs.json @@ -0,0 +1,24 @@ +{ + "autoreboot": 1, + "cidr": "192.168.1.114/24", + "country": "at", + "dns": "192.168.1.254", + "domain": "testinstall", + "disk_selection": { +"6": "6", +"7": "7" + }, + "lvm_auto_rename": 1, + "filesys": "btrfs (RAID1)", + "gateway": "192.168.1.1", + "hdsize": 80.0, + "hostname": "pveauto", + "keymap": "de", + "mailto": "mail@no.invalid", + "mngmt_nic": "eno1", + "password": "123456", + "timezone": "Europe/Vienna", + "btrfs_opts": { +"compress": "zlib" + } +} diff --git a/proxmox-auto-installer/tests/resources/parse_answer/btrfs.toml b/proxmox-auto-installer/tests/resources/parse_answer/btrfs.toml new file mode 100644 index 000..8fcd27d --- /dev/null +++ b/proxmox-auto-installer/tests/resources/parse_answer/btrfs.toml @@ -0,0 +1,17 @@ +[global] +keyboard = "de" +country = "at" +fqdn = "pveauto.testinstall" +mailto = "mail@no.invalid" +timezone = "Europe/Vienna" +root_password = "123456" + +[network] +source = "from-dhcp" + +[disk-setup] +filesystem = "btrfs" +btrfs.raid = "raid1" +btrfs.compress = "zlib" +btrfs.hdsize = 80 +disk_list = ["sda", "sdb"] -- 2.44.0 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer 2/6] fix #5250: install: write btrfs `compress` option to fstab
`compress` instead of `compress-force` is used, as the latter can have unindented (performance) implications, as the name implies. That would be neither expected by users nor should such a decision made without the user explicitly opting for it. Others do the same, e.g. the installer for RedHat/Fedora systems (aka. Anaconda) opts for `compress` also. And if Fedora uses it for their systems, it's fine for us too. Signed-off-by: Christoph Heiss --- Proxmox/Install.pm | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm index c0f8955..f3bc5aa 100644 --- a/Proxmox/Install.pm +++ b/Proxmox/Install.pm @@ -1011,7 +1011,13 @@ sub extract_data { die "unable to detect FS UUID" if !defined($fsuuid); - $fstab .= "UUID=$fsuuid / btrfs defaults 0 1\n"; + my $btrfs_opts = Proxmox::Install::Config::get_btrfs_opt(); + + my $mountopts = 'defaults'; + $mountopts .= ",compress=$btrfs_opts->{compress}" + if $btrfs_opts->{compress} ne 'off'; + + $fstab .= "UUID=$fsuuid / btrfs $mountopts 0 1\n"; } else { my $root_mountopt = $fssetup->{$filesys}->{root_mountopt} || 'defaults'; $fstab .= "$rootdev / $filesys ${root_mountopt} 0 1\n"; -- 2.44.0 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer 3/6] fix #5250: proxinstall: expose new btrfs `compress` option
Signed-off-by: Christoph Heiss --- Proxmox/Install.pm | 7 +-- proxinstall| 15 +++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm index f3bc5aa..60f38e5 100644 --- a/Proxmox/Install.pm +++ b/Proxmox/Install.pm @@ -1014,8 +1014,11 @@ sub extract_data { my $btrfs_opts = Proxmox::Install::Config::get_btrfs_opt(); my $mountopts = 'defaults'; - $mountopts .= ",compress=$btrfs_opts->{compress}" - if $btrfs_opts->{compress} ne 'off'; + if ($btrfs_opts->{compress} eq 'on') { + $mountopts .= ',compress'; + } elsif ($btrfs_opts->{compress} ne 'off') { + $mountopts .= ",compress=$btrfs_opts->{compress}"; + } $fstab .= "UUID=$fsuuid / btrfs $mountopts 0 1\n"; } else { diff --git a/proxinstall b/proxinstall index a6a4cfb..7180fe6 100755 --- a/proxinstall +++ b/proxinstall @@ -1160,6 +1160,21 @@ my $create_raid_advanced_grid = sub { my $create_btrfs_raid_advanced_grid = sub { my ($hdsize_btn) = @_; my $labeled_widgets = []; + +my $combo_compress = Gtk3::ComboBoxText->new(); +$combo_compress->set_tooltip_text("btrfs compression algorithm for boot volume"); +my $comp_opts = ["on", "off", "zlib", "lzo", "zstd"]; +foreach my $opt (@$comp_opts) { + $combo_compress->append($opt, $opt); +} +my $compress = Proxmox::Install::Config::get_btrfs_opt('compress') // 'off'; +$combo_compress->set_active_id($compress); +$combo_compress->signal_connect (changed => sub { + my $w = shift; + Proxmox::Install::Config::set_btrfs_opt('compress', $w->get_active_text()); +}); +push @$labeled_widgets, ['compress', $combo_compress]; + push @$labeled_widgets, ['hdsize', $hdsize_btn, 'GB']; return $create_label_widget_grid->($labeled_widgets);; }; -- 2.44.0 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH proxmox-firewall 1/1] firewall: properly reject ipv6 traffic
ICMPv6 has different message types for rejecting traffic. With ICMP we used host-prohibited as rejection type, which doesn't exist in ICMPv6. Add an additional rule for IPv6, so it uses admin-prohibited. Signed-off-by: Stefan Hanreich --- proxmox-firewall/resources/proxmox-firewall.nft | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/proxmox-firewall/resources/proxmox-firewall.nft b/proxmox-firewall/resources/proxmox-firewall.nft index f36bf3b..0a220bf 100644 --- a/proxmox-firewall/resources/proxmox-firewall.nft +++ b/proxmox-firewall/resources/proxmox-firewall.nft @@ -75,8 +75,9 @@ table inet proxmox-firewall { ip saddr 224.0.0.0/4 drop meta l4proto tcp reject with tcp reset -meta l4proto icmp reject with icmp type port-unreachable +meta l4proto icmp reject with icmpx type port-unreachable reject with icmp type host-prohibited +reject with icmpv6 type admin-prohibited } set v4-dc/management { @@ -289,8 +290,9 @@ table bridge proxmox-firewall-guests { ip saddr 224.0.0.0/4 drop meta l4proto tcp reject with tcp reset -meta l4proto icmp reject with icmp type port-unreachable +meta l4proto icmp reject with icmpx type port-unreachable reject with icmp type host-prohibited +reject with icmpv6 type admin-prohibited } chain after-vm-in { -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH installer v2 0/4] assistant: clean up glob patterns & regexes
did some tests on my local machine with the `device-info` and `device-match` subcommands to list and match against identifiers of disks and my NIC. Thanks for taking the time to improve this code. consider this series: Reviewed-By: Aaron Lauterer Tested-By: Aaron Lauterer On 2024-05-13 11:49, Christoph Heiss wrote: The proxmox-auto-install-assistant uses - glob patterns for disk matching, which can be pre-compiled for efficiency - regexes for udev property matching, which can be simplified by some simple prefix matching & splitting on = The latter also significantly reduces binary size due to the removing the regex dependency, for details see patch #4. Overall no functional changes in this series. v1: https://lists.proxmox.com/pipermail/pve-devel/2024-May/063802.html Changes v1 -> v2: * rebased on latest master Christoph Heiss (4): tree-wide: run rustfmt, fix clippy warnings assistant: drop unused `log` dependency assistant: pre-compile ignored block device patterns assistant: avoid regex for simple prefix matching proxmox-auto-install-assistant/Cargo.toml | 2 - proxmox-auto-install-assistant/src/main.rs| 75 --- proxmox-auto-installer/tests/parse-answer.rs | 14 ++-- .../src/fetch_plugins/partition.rs| 10 +-- 4 files changed, 45 insertions(+), 56 deletions(-) -- 2.44.0 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH installer 0/4] assistant: clean up glob patterns & regexes
On Mon, May 13, 2024 at 11:18:51AM +0200, Aaron Lauterer wrote: > it seems these patches don't apply anymore. could you please do a rebase on > current master? Sure, thanks for the notice! Seems 1a01a01 ("assistant: keep prepared iso bootable on uefi with flash drives") "broke" it, due to an import statement change :') v2: https://lists.proxmox.com/pipermail/pve-devel/2024-May/063833.html > > On 2024-05-07 15:21, Christoph Heiss wrote: > > The proxmox-auto-install-assistant uses > >- glob patterns for disk matching, which can be pre-compiled for > > efficiency > >- regexes for udev property matching, which can be simplified by some > > simple prefix matching & splitting on = > > > > The latter also significantly reduces binary size due to the removing > > the regex dependency, for details see patch #4. > > > > Overall no functional changes in this series. > > > > Christoph Heiss (4): > >tree-wide: run rustfmt, fix clippy warnings > >assistant: drop unused `log` dependency > >assistant: pre-compile ignored block device patterns > >assistant: avoid regex for simple prefix matching > > > > proxmox-auto-install-assistant/Cargo.toml | 2 - > > proxmox-auto-install-assistant/src/main.rs| 75 --- > > proxmox-auto-installer/tests/parse-answer.rs | 14 ++-- > > .../src/fetch_plugins/partition.rs| 10 +-- > > 4 files changed, 45 insertions(+), 56 deletions(-) > > > > -- > > 2.44.0 > > > > > > > > ___ > > pve-devel mailing list > > pve-devel@lists.proxmox.com > > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > > > ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer v2 4/4] assistant: avoid regex for simple prefix matching
udev properties are very easy to parse and can be done by doing a line-based scan and matching the prefix, splitting once for properties. Avoids the use of regexes and signicantly reduces binary size by about -38%(!). Tested by comparing the output of `proxmox-auto-install-assistant device-info`, running it before and after the changes. Stripped binary size for release builds: before: 3103104 bytes ~ 2.96MiB after: 1935744 bytes ~ 1.85MiB textdata bss dec hex filename 2906765 187144 537 3094446 2f37ae proxmox-auto-install-assistant-before 1871090 55736 497 1927323 1d689b proxmox-auto-install-assistant-after No functional changes. Signed-off-by: Christoph Heiss --- Changes v1 -> v2: * rebased on latest master proxmox-auto-install-assistant/Cargo.toml | 1 - proxmox-auto-install-assistant/src/main.rs | 57 +- 2 files changed, 24 insertions(+), 34 deletions(-) diff --git a/proxmox-auto-install-assistant/Cargo.toml b/proxmox-auto-install-assistant/Cargo.toml index 0286c80..766b445 100644 --- a/proxmox-auto-install-assistant/Cargo.toml +++ b/proxmox-auto-install-assistant/Cargo.toml @@ -15,7 +15,6 @@ anyhow = "1.0" clap = { version = "4.0", features = ["derive"] } glob = "0.3" proxmox-auto-installer = { path = "../proxmox-auto-installer" } -regex = "1.7" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" toml = "0.7" diff --git a/proxmox-auto-install-assistant/src/main.rs b/proxmox-auto-install-assistant/src/main.rs index 790dbc7..7c0b0c6 100644 --- a/proxmox-auto-install-assistant/src/main.rs +++ b/proxmox-auto-install-assistant/src/main.rs @@ -1,7 +1,6 @@ use anyhow::{bail, format_err, Result}; use clap::{Args, Parser, Subcommand, ValueEnum}; use glob::Pattern; -use regex::Regex; use serde::Serialize; use std::{ collections::BTreeMap, @@ -439,13 +438,9 @@ fn get_disks() -> Result>> { Pattern::new("sr[0-9]*")?, ]; -// compile Regex here once and not inside the loop -let re_disk = Regex::new(r"(?m)^E: DEVTYPE=disk")?; -let re_cdrom = Regex::new(r"(?m)^E: ID_CDROM")?; -let re_iso9660 = Regex::new(r"(?m)^E: ID_FS_TYPE=iso9660")?; - -let re_name = Regex::new(r"(?m)^N: (.*)$")?; -let re_props = Regex::new(r"(?m)^E: ([^=]+)=(.*)$")?; +const PROP_DEVTYP_PREFIX: = "E: DEVTYPE="; +const PROP_CDROM: = "E: ID_CDROM"; +const PROP_ISO9660_FS: = "E: ID_FS_TYPE=iso9660"; let mut disks: BTreeMap> = BTreeMap::new(); @@ -467,30 +462,27 @@ fn get_disks() -> Result>> { } }; -if !re_disk.is_match() { -continue 'outer; -}; -if re_cdrom.is_match() { -continue 'outer; -}; -if re_iso9660.is_match() { -continue 'outer; -}; - let mut name = filename; -if let Some(cap) = re_name.captures() { -if let Some(res) = cap.get(1) { -name = String::from(res.as_str()); +let mut udev_props: BTreeMap = BTreeMap::new(); +for line in output.lines() { +if let Some(prop) = line.strip_prefix(PROP_DEVTYP_PREFIX) { +if prop != "disk" { +continue 'outer; +} } -} -let mut udev_props: BTreeMap = BTreeMap::new(); +if line.starts_with(PROP_CDROM) || line.starts_with(PROP_ISO9660_FS) { +continue 'outer; +} -for line in output.lines() { -if let Some(caps) = re_props.captures(line) { -let key = String::from(caps.get(1).unwrap().as_str()); -let value = String::from(caps.get(2).unwrap().as_str()); -udev_props.insert(key, value); +if let Some(prop) = line.strip_prefix("N: ") { +name = prop.to_owned(); +}; + +if let Some(prop) = line.strip_prefix("E: ") { +if let Some((key, val)) = prop.split_once('=') { +udev_props.insert(key.to_owned(), val.to_owned()); +} } } @@ -500,7 +492,6 @@ fn get_disks() -> Result>> { } fn get_nics() -> Result>> { -let re_props = Regex::new(r"(?m)^E: (.*)=(.*)$")?; let mut nics: BTreeMap> = BTreeMap::new(); let links = get_nic_list()?; @@ -518,10 +509,10 @@ fn get_nics() -> Result>> { let mut udev_props: BTreeMap = BTreeMap::new(); for line in output.lines() { -if let Some(caps) = re_props.captures(line) { -let key = String::from(caps.get(1).unwrap().as_str()); -let value = String::from(caps.get(2).unwrap().as_str()); -udev_props.insert(key, value); +if let Some(prop) = line.strip_prefix("E: ") { +if let Some((key, val)) = prop.split_once('=') { +udev_props.insert(key.to_owned(), val.to_owned()); +} }
[pve-devel] [PATCH installer v2 3/4] assistant: pre-compile ignored block device patterns
No functional changes. Signed-off-by: Christoph Heiss --- Changes v1 -> v2: * no changes proxmox-auto-install-assistant/src/main.rs | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/proxmox-auto-install-assistant/src/main.rs b/proxmox-auto-install-assistant/src/main.rs index 1447175..790dbc7 100644 --- a/proxmox-auto-install-assistant/src/main.rs +++ b/proxmox-auto-install-assistant/src/main.rs @@ -430,13 +430,13 @@ fn get_iso_uuid(iso: ) -> Result { } fn get_disks() -> Result>> { -let unwantend_block_devs = vec![ -"ram[0-9]*", -"loop[0-9]*", -"md[0-9]*", -"dm-*", -"fd[0-9]*", -"sr[0-9]*", +let unwanted_block_devs = [ +Pattern::new("ram[0-9]*")?, +Pattern::new("loop[0-9]*")?, +Pattern::new("md[0-9]*")?, +Pattern::new("dm-*")?, +Pattern::new("fd[0-9]*")?, +Pattern::new("sr[0-9]*")?, ]; // compile Regex here once and not inside the loop @@ -453,8 +453,8 @@ fn get_disks() -> Result>> { let entry = entry.unwrap(); let filename = entry.file_name().into_string().unwrap(); -for p in _block_devs { -if Pattern::new(p)?.matches() { +for p in _block_devs { +if p.matches() { continue 'outer; } } -- 2.44.0 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer v2 2/4] assistant: drop unused `log` dependency
No functional changes. Signed-off-by: Christoph Heiss --- Changes v1 -> v2: * no changes proxmox-auto-install-assistant/Cargo.toml | 1 - 1 file changed, 1 deletion(-) diff --git a/proxmox-auto-install-assistant/Cargo.toml b/proxmox-auto-install-assistant/Cargo.toml index eaca7f8..0286c80 100644 --- a/proxmox-auto-install-assistant/Cargo.toml +++ b/proxmox-auto-install-assistant/Cargo.toml @@ -14,7 +14,6 @@ homepage = "https://www.proxmox.com; anyhow = "1.0" clap = { version = "4.0", features = ["derive"] } glob = "0.3" -log = "0.4.20" proxmox-auto-installer = { path = "../proxmox-auto-installer" } regex = "1.7" serde = { version = "1.0", features = ["derive"] } -- 2.44.0 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer v2 0/4] assistant: clean up glob patterns & regexes
The proxmox-auto-install-assistant uses - glob patterns for disk matching, which can be pre-compiled for efficiency - regexes for udev property matching, which can be simplified by some simple prefix matching & splitting on = The latter also significantly reduces binary size due to the removing the regex dependency, for details see patch #4. Overall no functional changes in this series. v1: https://lists.proxmox.com/pipermail/pve-devel/2024-May/063802.html Changes v1 -> v2: * rebased on latest master Christoph Heiss (4): tree-wide: run rustfmt, fix clippy warnings assistant: drop unused `log` dependency assistant: pre-compile ignored block device patterns assistant: avoid regex for simple prefix matching proxmox-auto-install-assistant/Cargo.toml | 2 - proxmox-auto-install-assistant/src/main.rs| 75 --- proxmox-auto-installer/tests/parse-answer.rs | 14 ++-- .../src/fetch_plugins/partition.rs| 10 +-- 4 files changed, 45 insertions(+), 56 deletions(-) -- 2.44.0 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer v2 1/4] tree-wide: run rustfmt, fix clippy warnings
No functional changes. Signed-off-by: Christoph Heiss --- Changes v1 -> v2: * no changes proxmox-auto-installer/tests/parse-answer.rs | 14 +++--- .../src/fetch_plugins/partition.rs | 10 +- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/proxmox-auto-installer/tests/parse-answer.rs b/proxmox-auto-installer/tests/parse-answer.rs index 4014b6d..e77a769 100644 --- a/proxmox-auto-installer/tests/parse-answer.rs +++ b/proxmox-auto-installer/tests/parse-answer.rs @@ -1,4 +1,4 @@ -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use serde_json::Value; use std::fs; @@ -24,9 +24,9 @@ fn get_answer(path: PathBuf) -> Result { Ok(answer) } -fn setup_test_basic(path: ) -> (SetupInfo, LocaleInfo, RuntimeInfo, UdevInfo) { +fn setup_test_basic(path: ) -> (SetupInfo, LocaleInfo, RuntimeInfo, UdevInfo) { let installer_info: SetupInfo = { -let mut path = path.clone(); +let mut path = path.to_path_buf(); path.push("iso-info.json"); read_json() @@ -35,7 +35,7 @@ fn setup_test_basic(path: ) -> (SetupInfo, LocaleInfo, RuntimeInfo, Udev }; let locale_info = { -let mut path = path.clone(); +let mut path = path.to_path_buf(); path.push("locales.json"); read_json() @@ -44,7 +44,7 @@ fn setup_test_basic(path: ) -> (SetupInfo, LocaleInfo, RuntimeInfo, Udev }; let mut runtime_info: RuntimeInfo = { -let mut path = path.clone(); +let mut path = path.to_path_buf(); path.push("run-env-info.json"); read_json() @@ -53,7 +53,7 @@ fn setup_test_basic(path: ) -> (SetupInfo, LocaleInfo, RuntimeInfo, Udev }; let udev_info: UdevInfo = { -let mut path = path.clone(); +let mut path = path.to_path_buf(); path.push("run-env-udev.json"); read_json() @@ -71,7 +71,7 @@ fn setup_test_basic(path: ) -> (SetupInfo, LocaleInfo, RuntimeInfo, Udev fn test_parse_answers() { let path = get_test_resource_path().unwrap(); let (setup_info, locales, runtime_info, udev_info) = setup_test_basic(); -let mut tests_path = path.clone(); +let mut tests_path = path; tests_path.push("parse_answer"); let test_dir = fs::read_dir(tests_path.clone()).unwrap(); diff --git a/proxmox-fetch-answer/src/fetch_plugins/partition.rs b/proxmox-fetch-answer/src/fetch_plugins/partition.rs index 0479c8f..7213493 100644 --- a/proxmox-fetch-answer/src/fetch_plugins/partition.rs +++ b/proxmox-fetch-answer/src/fetch_plugins/partition.rs @@ -31,7 +31,7 @@ impl FetchFromPartition { } fn path_exists_logged(file_name: , search_path: ) -> Option { -let path = Path::new(search_path).join(_name); +let path = Path::new(search_path).join(file_name); info!("Testing partition search path {path:?}"); match path.try_exists() { Ok(true) => Some(path), @@ -51,14 +51,14 @@ fn path_exists_logged(file_name: , search_path: ) -> Option { fn scan_partlabels(partlabel: , search_path: ) -> Result { let partlabel_upper_case = partlabel.to_uppercase(); if let Some(path) = path_exists_logged(_upper_case, search_path) { -info!("Found partition with label '{partlabel_upper_case}'"); -return Ok(path); +info!("Found partition with label '{partlabel_upper_case}'"); +return Ok(path); } let partlabel_lower_case = partlabel.to_lowercase(); if let Some(path) = path_exists_logged(_lower_case, search_path) { -info!("Found partition with label '{partlabel_lower_case}'"); -return Ok(path); +info!("Found partition with label '{partlabel_lower_case}'"); +return Ok(path); } bail!("Could not detect upper or lower case labels for '{partlabel}'"); -- 2.44.0 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH installer 0/4] assistant: clean up glob patterns & regexes
it seems these patches don't apply anymore. could you please do a rebase on current master? On 2024-05-07 15:21, Christoph Heiss wrote: The proxmox-auto-install-assistant uses - glob patterns for disk matching, which can be pre-compiled for efficiency - regexes for udev property matching, which can be simplified by some simple prefix matching & splitting on = The latter also significantly reduces binary size due to the removing the regex dependency, for details see patch #4. Overall no functional changes in this series. Christoph Heiss (4): tree-wide: run rustfmt, fix clippy warnings assistant: drop unused `log` dependency assistant: pre-compile ignored block device patterns assistant: avoid regex for simple prefix matching proxmox-auto-install-assistant/Cargo.toml | 2 - proxmox-auto-install-assistant/src/main.rs| 75 --- proxmox-auto-installer/tests/parse-answer.rs | 14 ++-- .../src/fetch_plugins/partition.rs| 10 +-- 4 files changed, 45 insertions(+), 56 deletions(-) -- 2.44.0 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel