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

[Tim Marx]

I agree with both of you regarding the regex, was at first on the same way, but I don't think its worth to create a new regex to only parse the "meta" lines and double the code only for that. I will change the existing regex a little bit, will make the groups optional and change the return properties to reflect that some of them maybe don't exist.

Regarding the commit:
I will send a single commit for the bugfix and depending on the end result maybe separate ones for clean up and enhancement.

thanks for the feedback!

> Thomas Lamprecht <t.lamprecht at proxmox.com> hat am 25. Oktober 2018 um 10:05 geschrieben:
> 
> 
> 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,
> > 
>

Best Regards,
Tim Marx


t.marx at proxmox.com
https://www.proxmox.com
_______________________________________________Proxmox Server Solutions GmbHBräuhausgasse 37, 1050 Vienna, AustriaCommercial register no.: FN 258879 fRegistration office: Handelsgericht Wien