Re: [pve-devel] [PATCH installer 1/3] low-level: add zfs module for retrieving importable zpool info

[Christoph Heiss <c.heiss@proxmox.com>]

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 0000000..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