[pve-devel] [PATCH storage] close #1949: storage zfs: enhanced zpool status parser

[Dominik Csapak]

works, a few comments inline


On 10/24/18 2:59 PM, Tim Marx wrote:
> changed the behavior which originally only parsed the used vdevs from a pool.
> with this revision spare-, cache- and log-disks are parsed as well.
> 
> *removed a unused local var vdevs
> *changed return value name to errors (I think that was a copy&past leftover)
> *changed usage of $1 to meaningful name $spaces

but this does not happen in the code?

> 
> we need to change the frontend as well, because of some missing fields when
> parsing spare disk (no state description.. ), I will do this in a follow up.
> Regarding the frontend, we could display some additional infos as well like
> errors and scan. As for now there is only a msg field for device specific
> messages. what do you think?

yes, it makes sense to display the info we have

> 
> Signed-off-by: Tim Marx <t.marx at proxmox.com>
> ---
>   PVE/API2/Disks/ZFS.pm | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm
> index 371cc5a..898dafc 100644
> --- a/PVE/API2/Disks/ZFS.pm
> +++ b/PVE/API2/Disks/ZFS.pm
> @@ -165,7 +165,7 @@ __PACKAGE__->register_method ({
>   		type => 'string',
>   		description => 'Information about the last/current scrub.',
>   	    },
> -	    scan => {
> +	    errors => {
>   		type => 'string',
>   		description => 'Information about the errors on the zpool.',
>   	    },
> @@ -213,7 +213,6 @@ __PACKAGE__->register_method ({
>   	my $pool = {
>   	    lvl => 0,
>   	};
> -	my $vdevs = [];
>   
>   	my $curfield;
>   	my $config = 0;
> @@ -233,10 +232,10 @@ __PACKAGE__->register_method ({
>   		$pool->{$curfield} .= " " . $1;
>   	    } elsif (!$config && $line =~ m/^\s*config:/) {
>   		$config = 1;
> -	    } elsif ($config && $line =~ m/^(\s+)(\S+)\s+(\S+)\s+(\S+)\s+(\S+)\s+(\S+)\s*(.*)$/) {
> +	    } elsif ($config && $line =~ m/^(\s+)(\S+)\s*(\S*)\s*(\S*)\s*(\S*)\s*(\S*)\s*(.*)$/) {

why are you making $3-$6 optional?
if you want to parse a line with another syntax, i would like to see
a 'elsif ( ... $line =~ m/newregex/ ) instead
or make a new non capturing group (?:) that is optional

in that case i would probably not put the fields
it does not have into the returned value
e.g. it does not make sens for a spare to have
cksum/read/write values at all, not even zero

ofc the return description must be adapted to make those optional

also, notice that a line like
' somefield: foo'

would now also get parsed by that regex
so if zpool status gets an output like that after the
config:
line, we would potentially parse things that makes no sense

>   		my ($space, $name, $state, $read, $write, $cksum, $msg) = ($1, $2, $3, $4, $5, $6, $7);
> -		if ($space  =~ m/^\t(\s+)$/) {
> -		    my $lvl= length($1)/2; # two spaces per level
> +		if ($name ne "NAME" and $name ne $param->{name}) {
> +		    my $lvl= length($space)/2; # two spaces per level
i guess this works, but lvl is now always a float (0.5, 1.5, 2.5,etc)
because you do not remove the leading \t

was this intended?

>   		    my $vdev = {
>   			name => $name,
>   			state => $state,
> 

while you are at it, could you move the 3 (out of context)
"push stack, cur;
push stack vdev" out of the if/elsif/else construct ?
it seems i did not notice that this is always the same
and moving it out of there makes the code much more readable