[pve-devel] [PATCH storage] fix #1895: use json for 'rbd ls -l' and 'rbd info'

[Thomas Lamprecht]

On 9/5/18 3:57 PM, Dominik Csapak wrote:
> since ceph changed the plain output format for 12.2.8
> we have to change the code anyway, and when were at it,
> we can change it to the (hopefully) more robust json output
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  PVE/Storage/RBDPlugin.pm | 75 +++++++++++++++++++++++++++---------------------
>  1 file changed, 43 insertions(+), 32 deletions(-)
> 
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index be88ad7..9536531 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -9,6 +9,7 @@ use PVE::Storage::Plugin;
>  use PVE::JSONSchema qw(get_standard_option);
>  use PVE::RADOS;
>  use PVE::Storage::CephTools;
> +use JSON;
>  
>  use base qw(PVE::Storage::Plugin);
>  
> @@ -153,25 +154,14 @@ sub run_rbd_command {
>  sub rbd_ls {
>      my ($scfg, $storeid) = @_;
>  
> -    my $cmd = &$rbd_cmd($scfg, $storeid, 'ls', '-l');
> +    my $cmd = &$rbd_cmd($scfg, $storeid, 'ls', '-l', '--format', 'json');
>      my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
>  
>      my $list = {};

list is, with your change, not directly used int the direct vicinity,
please move it below to the foreach where it's actually used.

> +    my $raw = "";
>  
>      my $parser = sub {
> -	my $line = shift;
> -
> -	if ($line =~  m/^((vm|base)-(\d+)-\S+)\s+(\d+)(k|M|G|T)\s((\S+)\/((vm|base)-\d+-\S+@\S+))?/) {
> -	    my ($image, $owner, $size, $unit, $parent) = ($1, $3, $4, $5, $8);
> -	    return if $image =~ /@/; #skip snapshots
> -
> -	    $list->{$pool}->{$image} = {
> -		name => $image,
> -		size => $size*$rbd_unittobytes->{$unit},
> -		parent => $parent,
> -		vmid => $owner
> -	    };
> -	}
> +	$raw .= shift;
>      };
>  
>      eval {
> @@ -180,7 +170,27 @@ sub rbd_ls {
>      my $err = $@;
>  
>      die $err if $err && $err !~ m/doesn't contain rbd images/ ;
> -  
> +
> +    my $result = JSON::decode_json($raw);

$raw can be an empty string, e.g., on a pool are no images yet, which decode_json
does not like at all:

> malformed JSON string, neither tag, array, object, number, string or atom

> +
> +    foreach my $l (@$result) {

what does $l mean? not really against one-character variable names but we normally use 
something short of what each element actually resembles or just 'e' or 'el', which
we normally use if we want to be short.

> +	next if defined($l->{snapshot});
> +
> +	my $parent;
> +	if ($l->{parent}) {
> +	    $parent = $l->{parent}->{image} . "@" . $l->{parent}->{snapshot};
> +	}
> +
> +	my ($owner) = $l->{image} =~ m/^(?:vm|base)-(\d+)-/;
> +
> +	$list->{$pool}->{$l->{image}} = {
                         ^^^^^^^^^^^
we try to avoid using a hash element as hash key directly and you need $l->{image} a
few times here so please add a my $image = $l->{image} above and use that one here.

> +	    name => $l->{image},
> +	    size => $l->{size},
> +	    parent => $parent,
> +	    vmid => $owner
> +	};
> +    }
> +
>      return $list;
>  }
>  
> @@ -189,36 +199,37 @@ sub rbd_volume_info {
>  
>      my $cmd = undef;
>  
> +    my @options = ('info', $volname, '--format', 'json');
>      if($snap){
> -       $cmd = &$rbd_cmd($scfg, $storeid, 'info', $volname, '--snap', $snap);
> -    }else{
> -       $cmd = &$rbd_cmd($scfg, $storeid, 'info', $volname);
> +	push @options, '--snap', $snap;
>      }
>  
> +    $cmd = &$rbd_cmd($scfg, $storeid, @options);
> +
>      my $size = undef;
>      my $parent = undef;
>      my $format = undef;
>      my $protected = undef;
>      my $features = undef;

with your changes, having those still here makes not much sense,
either move or just remove them

>  
> +    my $raw = "";
>      my $parser = sub {
> -	my $line = shift;
> -
> -	if ($line =~ m/size (\d+) (k|M|G|T)B in (\d+) objects/) {
> -	    $size = $1 * $rbd_unittobytes->{$2} if ($1);
> -	} elsif ($line =~ m/parent:\s(\S+)\/(\S+)/) {
> -	    $parent = $2;
> -	} elsif ($line =~ m/format:\s(\d+)/) {
> -	    $format = $1;
> -	} elsif ($line =~ m/protected:\s(\S+)/) {
> -	    $protected = 1 if $1 eq "True";
> -	} elsif ($line =~ m/features:\s(.+)/) {
> -	    $features = $1;
> -	}
> -
> +	$raw .= shift;
>      };
>  
>      run_rbd_command($cmd, errmsg => "rbd error", errfunc => sub {}, outfunc => $parser);
> +    my $r = JSON::decode_json($raw);

while it's not likely to get an empty string without an error from the command, with the rate
ceph changes output it may still make sense to have some general input validation!
And what's 'r'?

could be shortened to something alike:

    [...]
    my $raw = "";
    my $parser = sub { $raw .= shift };

    run_rbd_command($cmd, errmsg => "rbd error", errfunc => sub {}, outfunc => $parser);

    my $vol_info = $raw ? JSON::decode_json($raw) : {};

    if (my $parent = $vol_info->{parent}) {
        $vol_info->{parent} = $parent->{image} . '@' . $parent->{snapshot};
    }
    if ($vol_info->{protected}) {
        $vol_info->{protected} = 1 if $vol_info->{protected} eq "true";
    }

    return $vol_info->@{qw(size parent format protected features)};
}


> +    $size = $r->{size};
> +    $format = $r->{format};
> +    if ($r->{parent}) {
> +	$parent = $r->{parent}->{image}.'@'.$r->{parent}->{snapshot};
> +    }
> +    if ($r->{protected}) {
> +	$protected = 1 if $r->{protected} eq "true";
> +    }
> +    if ($r->{features}) {
> +	$features = join (', ', $r->{features});
> +    }
>  
>      return ($size, $parent, $format, $protected, $features);
>  }
>