[pve-devel] [PATCH v2 storage 1/3] close #1949: storage zfs: changed zpool command parser

[Tim Marx]

> Dominik Csapak <d.csapak at proxmox.com> hat am 5. November 2018 um 14:15 geschrieben:
> 
> 
> On 10/31/18 2:04 PM, Tim Marx wrote:
> > Signed-off-by: Tim Marx <t.marx at proxmox.com>
> > ---
> > 
> > changes since v1:
> > * changed regex matching groups
> > * changed $lvl back to int from float
> > * set vdev keys only if defined
> > 
> >   PVE/API2/Disks/ZFS.pm | 26 ++++++++++++--------------
> >   1 file changed, 12 insertions(+), 14 deletions(-)
> > 
> > diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm
> > index 58a498d..4780bcd 100644
> > --- a/PVE/API2/Disks/ZFS.pm
> > +++ b/PVE/API2/Disks/ZFS.pm
> > @@ -233,39 +233,37 @@ __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*(.*)$/) {
> 
> mhmm works, but not exactly what i had in mind, more like:
> 
> (\s+)(\S+)(?:\s+(\S+)\s+(\S+)\s+(\S+)\s+(\S+))?\s*(.*)
> 
> this makes it so that state,read,write,cksum are all there, or none
> 
> any regex experts here (regarding performance/pitfalls/etc.) ? @all
> 

Yeah, if there is no other opinion I will change that. Seems more robust in case of a layout change thanks!

> >   		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/2; # two spaces per level
> 
> mhmm, i dont think it does what it you think it does (operator order)
> it now adds 1 to the length of the string
> 
> eg. "aaa" => 4
> 
> but even disegarding operator order it would mean that we get
> "aaa" => 5/2 => 2.5

In fact it adds only 1, because its always 1 tab + even spaces => always even
As long as the format stays the same, there shouldn't be any floats anymore.
Didn't cleaned that before the commit, was (length(..)+2)/2 before and just removed the brackets while testing, so I agree with you 2/2 for 1 is very odd haha.

Another approach would be to add another capturing group only for tabs. 

> rest looks good
> 
> >   		    my $vdev = {
> >   			name => $name,
> > -			state => $state,
> > -			read => $read + 0,
> > -			write => $write + 0,
> > -			cksum => $cksum + 0,
> >   			msg => $msg,
> >   			lvl => $lvl,
> >   		    };
> > -
> > +		
> > +		    $vdev->{state} = $state if defined($state);
> > +		    $vdev->{read} = $read + 0 if defined($read);
> > +		    $vdev->{write} = $write + 0 if defined($write);
> > +		    $vdev->{cksum} = $cksum + 0 if defined($cksum);
> > +		
> >   		    my $cur = pop @$stack;
> >   
> >   		    if ($lvl > $curlvl) {
> >   			$cur->{children} = [ $vdev ];
> > -			push @$stack, $cur;
> > -			push @$stack, $vdev;
> >   		    } elsif ($lvl == $curlvl) {
> >   			$cur = pop @$stack;
> >   			push @{$cur->{children}}, $vdev;
> > -			push @$stack, $cur;
> > -			push @$stack, $vdev;
> >   		    } else {
> >   			while ($lvl <= $cur->{lvl}) {
> >   			    $cur = pop @$stack;
> >   			}
> >   			push @{$cur->{children}}, $vdev;
> > -			push @$stack, $cur;
> > -			push @$stack, $vdev;
> >   		    }
> > +		
> > +		    push @$stack, $cur;
> > +		    push @$stack, $vdev;
> >   		    $curlvl = $lvl;
> >   		}
> >   	    }
> > 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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