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

[Dominik Csapak]

On 10/25/18 9:52 AM, Dominik Csapak wrote:
> 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
> 

disregard the point above, we detect something like ' foo: bar'  earlier
so this is not a problem

>>           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
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel