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

[Dominik Csapak]

On 11/6/18 9:52 AM, Tim Marx wrote:
> 
> 
>> 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.

ok, i guess what we really want is :

int(length($space)/2)

this makes it clear that it is an int, and produces the correct levels
1 -> 0
3 -> 1
5 -> 2
etc.

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