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

[Thomas Lamprecht]

It works looks mostly OK, but I agree with Dominik regarding the separate regex
check in it's own if branch.

Besides that some comment regarding changes batching:

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

In general good, but if you need to make such points in a commit message it's
a good sign for splitting the patch up. A patch should try to concentrate to
do one thing and one thing only, even if it often varies a bit where you set
the boundary for "one thing".

But in this case the s/scan/errors/ is it's ownm change and could get applied
already - it's a change which is small and clearly an error and merits it's
own commit.

$vdevs could be cleaned up together with the other stuff, this is a bit a grey
area, IMO, if you plan to do a general cleanup/refactoring then I'd do it there.

> 
> 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?
> 
> 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*(.*)$/) {
>  		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
>  		    my $vdev = {
>  			name => $name,
>  			state => $state,
>