[pve-devel] [PATCH v3 storage 1/2] fix #3633: pvesm: use print_api_result

[Dominik Csapak]

both patches LGTM, commit message could have used some more info
(like which fields were added, etc.) but no big problem

Reviewed-by: Dominik Csapak <d.csapak at proxmox.com>
Tested-by: Dominik Csapak<d.csapak at proxmox.com>

On 10/21/21 14:49, Lorenz Stechauner wrote:
> Signed-off-by: Lorenz Stechauner <l.stechauner at proxmox.com>
> ---
>   PVE/CLI/pvesm.pm | 158 +++++++++--------------------------------------
>   1 file changed, 30 insertions(+), 128 deletions(-)
> 
> diff --git a/PVE/CLI/pvesm.pm b/PVE/CLI/pvesm.pm
> index 190de91..94847cd 100755
> --- a/PVE/CLI/pvesm.pm
> +++ b/PVE/CLI/pvesm.pm
> @@ -178,68 +178,6 @@ __PACKAGE__->register_method ({
>   	return;
>       }});
>   
> -my $print_content = sub {
> -    my ($list) = @_;
> -
> -    my ($maxlenname, $maxsize) = (0, 0);
> -    foreach my $info (@$list) {
> -	my $volid = $info->{volid};
> -	my $sidlen =  length ($volid);
> -	$maxlenname = $sidlen if $sidlen > $maxlenname;
> -	$maxsize = $info->{size} if ($info->{size} // 0) > $maxsize;
> -    }
> -    my $sizemaxdigits = length($maxsize);
> -
> -    my $basefmt = "%-${maxlenname}s %-7s %-9s %${sizemaxdigits}s";
> -    printf "$basefmt %s\n", "Volid", "Format", "Type", "Size", "VMID";
> -
> -    foreach my $info (@$list) {
> -	next if !$info->{vmid};
> -	my $volid = $info->{volid};
> -
> -	printf "$basefmt %d\n", $volid, $info->{format}, $info->{content}, $info->{size}, $info->{vmid};
> -    }
> -
> -    foreach my $info (sort { $a->{format} cmp $b->{format} } @$list) {
> -	next if $info->{vmid};
> -	my $volid = $info->{volid};
> -
> -	printf "$basefmt\n", $volid, $info->{format}, $info->{content}, $info->{size};
> -    }
> -};
> -
> -my $print_status = sub {
> -    my $res = shift;
> -
> -    my $maxlen = 0;
> -    foreach my $res (@$res) {
> -	my $storeid = $res->{storage};
> -	$maxlen = length ($storeid) if length ($storeid) > $maxlen;
> -    }
> -    $maxlen+=1;
> -
> -    printf "%-${maxlen}s %10s %10s %15s %15s %15s %8s\n", 'Name', 'Type',
> -	'Status', 'Total', 'Used', 'Available', '%';
> -
> -    foreach my $res (sort { $a->{storage} cmp $b->{storage} } @$res) {
> -	my $storeid = $res->{storage};
> -
> -	my $active = $res->{active} ? 'active' : 'inactive';
> -	my ($per, $per_fmt) = (0, '% 7.2f%%');
> -	$per = ($res->{used}*100)/$res->{total} if $res->{total} > 0;
> -
> -	if (!$res->{enabled}) {
> -	    $per = 'N/A';
> -	    $per_fmt = '% 8s';
> -	    $active = 'disabled';
> -	}
> -
> -	printf "%-${maxlen}s %10s %10s %15d %15d %15d $per_fmt\n", $storeid,
> -	    $res->{type}, $active, $res->{total}/1024, $res->{used}/1024,
> -	    $res->{avail}/1024, $per;
> -    }
> -};
> -
>   __PACKAGE__->register_method ({
>       name => 'export',
>       path => 'export',
> @@ -573,14 +511,26 @@ my $print_api_result = sub {
>       PVE::CLIFormatter::print_api_result($data, $schema, undef, $options);
>   };
>   
> +my $print_content = sub {
> +    my ($data, $schema, $options) = @_;
> +    my $order = [qw(volid format content size used vmid ctime parent encrypted verification notes)];
> +    PVE::CLIFormatter::print_api_result($data, $schema, $order, $options);
> +};
> +
> +my $print_status = sub {
> +    my ($data, $schema, $options) = @_;
> +    my $order = [qw(storage type active enabled shared total used avail used_fraction content)];
> +    PVE::CLIFormatter::print_api_result($data, $schema, $order, $options);
> +};
> +
>   our $cmddef = {
>       add => [ "PVE::API2::Storage::Config", 'create', ['type', 'storage'] ],
>       set => [ "PVE::API2::Storage::Config", 'update', ['storage'] ],
>       remove => [ "PVE::API2::Storage::Config", 'delete', ['storage'] ],
> -    status => [ "PVE::API2::Storage::Status", 'index', [],
> -		{ node => $nodename }, $print_status ],
> -    list => [ "PVE::API2::Storage::Content", 'index', ['storage'],
> -	      { node => $nodename }, $print_content ],
> +    status => [ "PVE::API2::Storage::Status", 'index', [], { node => $nodename },
> +	        $print_status, $PVE::RESTHandler::standard_output_options ],
> +    list => [ "PVE::API2::Storage::Content", 'index', ['storage'], { node => $nodename },
> +	      $print_content, $PVE::RESTHandler::standard_output_options ],
>       alloc => [ "PVE::API2::Storage::Content", 'create', ['storage', 'vmid', 'filename', 'size'],
>   	       { node => $nodename }, sub {
>   		   my $volid = shift;
> @@ -589,61 +539,18 @@ our $cmddef = {
>       free => [ "PVE::API2::Storage::Content", 'delete', ['volume'],
>   	      { node => $nodename } ],
>       scan => {
> -	nfs => [ "PVE::API2::Storage::Scan", 'nfsscan', ['server'], { node => $nodename }, sub  {
> -	    my $res = shift;
> -
> -	    my $maxlen = 0;
> -	    foreach my $rec (@$res) {
> -		my $len = length ($rec->{path});
> -		$maxlen = $len if $len > $maxlen;
> -	    }
> -	    foreach my $rec (@$res) {
> -		printf "%-${maxlen}s %s\n", $rec->{path}, $rec->{options};
> -	    }
> -	}],
> -	cifs => [ "PVE::API2::Storage::Scan", 'cifsscan', ['server'], { node => $nodename }, sub  {
> -	    my $res = shift;
> -
> -	    my $maxlen = 0;
> -	    foreach my $rec (@$res) {
> -		my $len = length ($rec->{share});
> -		$maxlen = $len if $len > $maxlen;
> -	    }
> -	    foreach my $rec (@$res) {
> -		printf "%-${maxlen}s %s\n", $rec->{share}, $rec->{description};
> -	    }
> -	}],
> -	glusterfs => [ "PVE::API2::Storage::Scan", 'glusterfsscan', ['server'], { node => $nodename }, sub  {
> -	    my $res = shift;
> -
> -	    foreach my $rec (@$res) {
> -		printf "%s\n", $rec->{volname};
> -	    }
> -	}],
> -	iscsi => [ "PVE::API2::Storage::Scan", 'iscsiscan', ['portal'], { node => $nodename }, sub  {
> -	    my $res = shift;
> -
> -	    my $maxlen = 0;
> -	    foreach my $rec (@$res) {
> -		my $len = length ($rec->{target});
> -		$maxlen = $len if $len > $maxlen;
> -	    }
> -	    foreach my $rec (@$res) {
> -		printf "%-${maxlen}s %s\n", $rec->{target}, $rec->{portal};
> -	    }
> -	}],
> -	lvm => [ "PVE::API2::Storage::Scan", 'lvmscan', [], { node => $nodename }, sub  {
> -	    my $res = shift;
> -	    foreach my $rec (@$res) {
> -		printf "$rec->{vg}\n";
> -	    }
> -	}],
> -	lvmthin => [ "PVE::API2::Storage::Scan", 'lvmthinscan', ['vg'], { node => $nodename }, sub  {
> -	    my $res = shift;
> -	    foreach my $rec (@$res) {
> -		printf "$rec->{lv}\n";
> -	    }
> -	}],
> +	nfs => [ "PVE::API2::Storage::Scan", 'nfsscan', ['server'], { node => $nodename },
> +		 $print_api_result, $PVE::RESTHandler::standard_output_options ],
> +	cifs => [ "PVE::API2::Storage::Scan", 'cifsscan', ['server'], { node => $nodename },
> +		  $print_api_result, $PVE::RESTHandler::standard_output_options ],
> +	glusterfs => [ "PVE::API2::Storage::Scan", 'glusterfsscan', ['server'], { node => $nodename },
> +		       $print_api_result, $PVE::RESTHandler::standard_output_options ],
> +	iscsi => [ "PVE::API2::Storage::Scan", 'iscsiscan', ['portal'], { node => $nodename },
> +		   $print_api_result, $PVE::RESTHandler::standard_output_options ],
> +	lvm => [ "PVE::API2::Storage::Scan", 'lvmscan', [], { node => $nodename },
> +		 $print_api_result, $PVE::RESTHandler::standard_output_options ],
> +	lvmthin => [ "PVE::API2::Storage::Scan", 'lvmthinscan', ['vg'], { node => $nodename },
> +		     $print_api_result, $PVE::RESTHandler::standard_output_options ],
>   	pbs => [
>   	    "PVE::API2::Storage::Scan",
>   	    'pbsscan',
> @@ -652,13 +559,8 @@ our $cmddef = {
>   	    $print_api_result,
>   	    $PVE::RESTHandler::standard_output_options,
>   	],
> -	zfs => [ "PVE::API2::Storage::Scan", 'zfsscan', [], { node => $nodename }, sub  {
> -	    my $res = shift;
> -
> -	    foreach my $rec (@$res) {
> -		 printf "$rec->{pool}\n";
> -	    }
> -	}],
> +	zfs => [ "PVE::API2::Storage::Scan", 'zfsscan', [], { node => $nodename },
> +		 $print_api_result, $PVE::RESTHandler::standard_output_options ],
>       },
>       nfsscan => { alias => 'scan nfs' },
>       cifsscan => { alias => 'scan cifs' },
>