Re: [pve-devel] [PATCH installer 1/3] low-level: add zfs module for retrieving importable zpool info
Thanks for the review! On Mon, Jul 08, 2024 at 04:24:16PM GMT, Aaron Lauterer wrote: > On 2024-05-16 12:28, Christoph Heiss wrote: > > [..] > > diff --git a/Proxmox/Sys/ZFS.pm b/Proxmox/Sys/ZFS.pm > > new file mode 100644 > > index 000..4c732ca > > --- /dev/null > > +++ b/Proxmox/Sys/ZFS.pm > > @@ -0,0 +1,43 @@ > > +package Proxmox::Sys::ZFS; > > + > > +use strict; > > +use warnings; > > + > > +use Proxmox::Sys::Command qw(run_command); > > + > > +use base qw(Exporter); > > +our @EXPORT_OK = qw(get_exported_pools); > > + > > Some of the flow in this function is difficult to understand without having > a sample of the text it is parsing. Could we have a small example, maybe > added as comment? > That could help people to see what is it trying to parse, even if they are > not too familiar with the expected output Makes sense, will do that! I'll also add some tests using verbatim outputs of `zfs import`, such that these can always be run. Probably easier than creating test pools and calling `zfs import` at test run time. > > > +my sub parse_pool_list { > > +my ($fh) = @_; > > + > > +my @pools; > > +my $pool = {}; # last found pool in output > > + > > +while (my $line = <$fh>) { > > + if ($line =~ /^\s+pool: (.+)$/) { > > + push @pools, $pool if %$pool; > > + $pool = { name => $1 }; > > + next; > > + } > > + > > + next if !%$pool; > > + > > + if ($line =~ /^\s*(id|state|status|action): (.+)$/) { > > + chomp($pool->{$1} = $2); > > + next; > > + } > > +} > > + > > +push @pools, $pool if %$pool; > > +return \@pools; > > not too sure, but we usually tend to use anonymous arrays, $pools = []; > then we could just return $pools > The downside is of course that we need to dereference it in all the other > places, AFAICT all the `push` lines: > push @$pools ... > > But IME this is more in line with how usually handle such code. Sure, I'll rewrite it in that style for v2. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH installer 1/3] low-level: add zfs module for retrieving importable zpool info
On 2024-05-16 12:28, Christoph Heiss wrote: Signed-off-by: Christoph Heiss --- Proxmox/Makefile | 1 + Proxmox/Sys/ZFS.pm| 43 ++ test/Makefile | 6 + test/zfs-get-pool-list.pl | 49 +++ 4 files changed, 99 insertions(+) create mode 100644 Proxmox/Sys/ZFS.pm create mode 100755 test/zfs-get-pool-list.pl diff --git a/Proxmox/Makefile b/Proxmox/Makefile index 9561d9b..035626b 100644 --- a/Proxmox/Makefile +++ b/Proxmox/Makefile @@ -17,6 +17,7 @@ PERL_MODULES=\ Sys/File.pm \ Sys/Net.pm \ Sys/Udev.pm \ +Sys/ZFS.pm \ UI.pm \ UI/Base.pm \ UI/Gtk3.pm \ diff --git a/Proxmox/Sys/ZFS.pm b/Proxmox/Sys/ZFS.pm new file mode 100644 index 000..4c732ca --- /dev/null +++ b/Proxmox/Sys/ZFS.pm @@ -0,0 +1,43 @@ +package Proxmox::Sys::ZFS; + +use strict; +use warnings; + +use Proxmox::Sys::Command qw(run_command); + +use base qw(Exporter); +our @EXPORT_OK = qw(get_exported_pools); + Some of the flow in this function is difficult to understand without having a sample of the text it is parsing. Could we have a small example, maybe added as comment? That could help people to see what is it trying to parse, even if they are not too familiar with the expected output +my sub parse_pool_list { +my ($fh) = @_; + +my @pools; +my $pool = {}; # last found pool in output + +while (my $line = <$fh>) { + if ($line =~ /^\s+pool: (.+)$/) { + push @pools, $pool if %$pool; + $pool = { name => $1 }; + next; + } + + next if !%$pool; + + if ($line =~ /^\s*(id|state|status|action): (.+)$/) { + chomp($pool->{$1} = $2); + next; + } +} + +push @pools, $pool if %$pool; +return \@pools; not too sure, but we usually tend to use anonymous arrays, $pools = []; then we could just return $pools The downside is of course that we need to dereference it in all the other places, AFAICT all the `push` lines: push @$pools ... But IME this is more in line with how usually handle such code. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer 1/3] low-level: add zfs module for retrieving importable zpool info
Signed-off-by: Christoph Heiss --- Proxmox/Makefile | 1 + Proxmox/Sys/ZFS.pm| 43 ++ test/Makefile | 6 + test/zfs-get-pool-list.pl | 49 +++ 4 files changed, 99 insertions(+) create mode 100644 Proxmox/Sys/ZFS.pm create mode 100755 test/zfs-get-pool-list.pl diff --git a/Proxmox/Makefile b/Proxmox/Makefile index 9561d9b..035626b 100644 --- a/Proxmox/Makefile +++ b/Proxmox/Makefile @@ -17,6 +17,7 @@ PERL_MODULES=\ Sys/File.pm \ Sys/Net.pm \ Sys/Udev.pm \ +Sys/ZFS.pm \ UI.pm \ UI/Base.pm \ UI/Gtk3.pm \ diff --git a/Proxmox/Sys/ZFS.pm b/Proxmox/Sys/ZFS.pm new file mode 100644 index 000..4c732ca --- /dev/null +++ b/Proxmox/Sys/ZFS.pm @@ -0,0 +1,43 @@ +package Proxmox::Sys::ZFS; + +use strict; +use warnings; + +use Proxmox::Sys::Command qw(run_command); + +use base qw(Exporter); +our @EXPORT_OK = qw(get_exported_pools); + +my sub parse_pool_list { +my ($fh) = @_; + +my @pools; +my $pool = {}; # last found pool in output + +while (my $line = <$fh>) { + if ($line =~ /^\s+pool: (.+)$/) { + push @pools, $pool if %$pool; + $pool = { name => $1 }; + next; + } + + next if !%$pool; + + if ($line =~ /^\s*(id|state|status|action): (.+)$/) { + chomp($pool->{$1} = $2); + next; + } +} + +push @pools, $pool if %$pool; +return \@pools; +} + +sub get_exported_pools { +my $raw = run_command(['zpool', 'import']); +open (my $fh, '<', \$raw) or die 'failed to open in-memory stream'; + +return parse_pool_list($fh); +} + +1; diff --git a/test/Makefile b/test/Makefile index 99bf14e..2d9decc 100644 --- a/test/Makefile +++ b/test/Makefile @@ -2,6 +2,7 @@ all: export PERLLIB=.. +# test-zfs-get-pool-list is not run by default, due to requiring root access .PHONY: check check: test-zfs-arc-max test-run-command test-parse-fqdn test-ui2-stdio @@ -9,6 +10,7 @@ check: test-zfs-arc-max test-run-command test-parse-fqdn test-ui2-stdio test-zfs-arc-max: ./zfs-arc-max.pl +.PHONY: test-run-command test-run-command: ./run-command.pl @@ -19,3 +21,7 @@ test-parse-fqdn: .PHONY: test-ui2-stdio test-ui2-stdio: ./ui2-stdio.pl + +.PHONY: test-zfs-get-pool-list +test-zfs-get-pool-list: + ./zfs-get-pool-list.pl diff --git a/test/zfs-get-pool-list.pl b/test/zfs-get-pool-list.pl new file mode 100755 index 000..072da2f --- /dev/null +++ b/test/zfs-get-pool-list.pl @@ -0,0 +1,49 @@ +#!/usr/bin/perl + +use strict; +use warnings; + +use File::Temp; +use Test::More tests => 6; + +use Proxmox::Sys::Command qw(syscmd run_command); +use Proxmox::Sys::ZFS; +use Proxmox::UI; + +my $log_file = File::Temp->new(); +Proxmox::Log::init($log_file->filename); + +Proxmox::UI::init_stdio(); + +our $POOL_PREFIX = 'pve-installer'; +my $pools = { 'test-pool1' => {}, 'test-pool2' => {} }; + +foreach (keys %$pools) { +my $f = File::Temp->new(); +print "$_: $f\n"; + +syscmd("truncate -s 1G $f"); +my $dev = run_command("losetup --find --show $f"); + +syscmd("zpool create $POOL_PREFIX-$_ $dev"); +syscmd("zpool export $POOL_PREFIX-$_"); + +$pools->{$_} = { + file => $f, + dev => $dev, +}; +} + +my $exported = Proxmox::Sys::ZFS::get_exported_pools(); +while (my ($name, $info) = each %$pools) { +my ($p) = grep { $_->{name} eq "$POOL_PREFIX-$name" } @$exported; +ok(defined($p), "pool $name was found"); +is($p->{state}, 'ONLINE', "pool $name is online"); +is($p->{action}, 'The pool can be imported using its name or numeric identifier.', + "pool $name can be imported"); +} + +keys %$pools; +while (my ($name, $info) = each %$pools) { +syscmd("losetup --detach $info->{dev}"); +} -- 2.44.0 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel