Re: [pve-devel] [PATCH v2 storage 2/2] smartctl: use json parsing

[Oguz Bektas <o.bektas@proxmox.com>]

thanks for reviewing! :)


On Mon, May 10, 2021 at 02:21:12PM +0200, Dominik Csapak wrote:
> some comments inline
> 
> >       }
> > -    my $cmd = [$SMARTCTL, '-H'];
> > -    push @$cmd, '-A', '-f', 'brief' if !$healthonly;
> > +    my $cmd = [$SMARTCTL, '-j', '-H'];
> > +    push @$cmd, '-Afbrief' if !$healthonly;
> 
> any particular reason to change this line? i think
> it makes it harder to read what flags we give
> (-Afbrief vs -A -f brief)

i think my idea was that the cli parameter count for the
disklist_test.pm would be reduced -- but i notice that actually this
doesn't bring anything so i suppose we can leave this part out.

> > +	if ($format eq 'text') {
> > +	    foreach my $key (sort keys %{$nvme_info}) {
> > +		my $value = $nvme_info->{$key};
> > +		# some fields can also be arrays
> > +		# e.g. temperature_sensor
> > +		if (ref($value) eq 'ARRAY') {
> > +		    while (my $index = each(@$value)) {
> > +			$add_key->("${key}_${index}", $value->[$index]);
> > +		    }
> 
> while this is ok, we probably could also do a
> 'pretty-print' json output here in case $value is not a scalar
> 
> that way we would also catch (potential) objects, not only arrays
> (though i do not know if that can happen)

hmm yes, haven't seen any other examples besides that array case, but i will look
into it.


 > +	my @sorted_attributes = sort { $a->{id} <=> $b->{id} } @{$smartdata->{attributes}};
> > +	@{$smartdata->{attributes}} = @sorted_attributes;
> > +    }
> > -    # bit 0 and 1 mark an severe smartctl error
> > -    # all others are for disk status, so ignore them
> > -    # see smartctl(8)
> > -    if ((defined($returncode) && ($returncode & 0b00000011)) || $err) {
> > +    if ($returncode & 0b00000011 || $err) {
> 
> again, any reason to change this? especially the comment about the return
> code ?

the comment was removed by mistake, sorry!

i was also thinking whether it's acceptable to take the exit code from
the json result (although then we might have to handle for potential
errors during parsing, so it might be better to keep it as before)