[pve-devel] [PATCH cd-builder 2/2] fix #1999: create a tree view for qm listsnapshot

[Thomas Lamprecht]

On 5/3/19 11:48 AM, Rhonda D'Vine wrote:
> The look of the tree is based on the GUI variant, so that we have a
> consistent output when run multiple times, too.
> 

in principle ok, comments to implementation details below, thanks!

> Signed-off-by: Rhonda D'Vine <rhonda at proxmox.com>
> ---
>  PVE/CLI/qm.pm | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 59 insertions(+), 4 deletions(-)
> 
> diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
> index fc06fa5..4d288df 100755
> --- a/PVE/CLI/qm.pm
> +++ b/PVE/CLI/qm.pm
> @@ -922,12 +922,67 @@ our $cmddef = {
>      listsnapshot => [ "PVE::API2::Qemu", 'snapshot_list', ['vmid'], { node => $nodename },
>  		    sub {
>  			my $res = shift;
> -			foreach my $e (sort { $a->{snaptime} ? ( $b->{snaptime} ? ($a->{snaptime} <=> $b->{snaptime}) : -1 ) : 1 } @$res) {
> -			    my $headline = $e->{description} || 'no-description';
> -			    $headline =~ s/\n.*//sg;
> +
> +			# "our" scope for the snaptimesort function
> +			our $snapshots;

why? why not just do:

my $snapshottree;
$snapshottree = sub {
   # outer scope available here...
}

$snapshottree->($prefix, $root, $snapshots);

> +			foreach my $e (@$res) {
> +			    $snapshots->{$e->{name}} = $e;
> +			}

maybe:
my $snapshots = { map { $_->{name} => $_ } @$res };

is a bit more concise (and faster, not that that would matter here).

> +
> +			my $root;
> +			foreach my $e (@$res) {
> +			    if ($e->{parent}) {
> +				push @{$snapshots->{$e->{parent}}->{children}}, $e->{name};

we try to avoid hash access in hash access, maybe:

if (my $parent = $e->{parent}) {
	push @{$snapshots->{$parent}->{children}}, $e->{name}};
}

(but this one is a bit borderline, IMO)

> +			    } else {
> +				$root = $e->{name};
> +			    }
> +			}
> +
> +			my $prefix = '`->';
> +
> +			# sort the elements by snaptime - with "current" (no snaptime) highest
> +			sub snaptimesort {
> +			    if ($snapshots->{$a}->{snaptime}) {
> +				if ($snapshots->{$b}->{snaptime}) {
> +				    return $snapshots->{$a}->{snaptime} <=> $snapshots->{$b}->{snaptime};
> +				} else {
> +				    return -1;
> +				}
> +			    } else {
> +				return 1;
> +			    }

ah here it is, but see proposal for:
https://pve.proxmox.com/pipermail/pve-devel/2019-May/036879.html
(naturally with the s/ret1urn/return/ typo fixed ;) )

And ensure if we want "defined" or "truthy" check here.

> +			}
> +
> +			# recursion function for displaying the tree
> +			sub snapshottree {
> +			    my ($prefix, $root, $snapshots) = @_;
> +			    my $e = $snapshots->{$root};
> +
> +			    my $description = $e->{description} || 'no-description';

instead of || use // as one could set a zero string as description "0" which
would then match as "falsy" here.

> +			    $description =~ s/\n.*//sg;

you only want to get the first line or? Maybe this (untested one) is a bit faster:
($description) =~ m/(.*)$/m;

as it can exit earlier (but not sure, perl could optimize yours possibly).

>  			    my $parent = $e->{parent} // 'no-parent';
> -			    printf("%-20s %-20s %s\n", $e->{name}, $parent, $headline);
> +
> +			    # create the timestamp string
> +			    my $timestring = "";
> +			    if (defined $e->{snaptime}) {
> +				my ($sec, $min, $hour, $mday, $mon, $year) = localtime($e->{snaptime});
> +				$year += 1900;
> +				$mon++;
> +				$timestring = sprintf("%04d-%02d-%02d %02d:%02d:%02d", $year, $mon, $mday, $hour, $min, $sec);

See a recent comment on such constructs for time:
https://pve.proxmox.com/pipermail/pve-devel/2019-April/036592.html
also
https://pve.proxmox.com/pipermail/pve-devel/2019-April/036654.html

so:
strftime("%F %H:%M:%S", localtime($e->{snaptime}));

(only works if Time::localtime is not included and overwrites perl's internal one)


> +			    }
> +
> +			    # for aligning the description
> +			    my $len = 30 - length($prefix);
> +			    printf("%s %-${len}s %-23s %s\n", $prefix, $root, $timestring, $description);
> +			    if ($e->{children}) {
> +				$prefix = "    $prefix";
> +				foreach my $child (sort snaptimesort @{$e->{children}}) {
> +				    snapshottree ($prefix, $child, $snapshots);
> +				}
> +			    }
>  			}
> +
> +			snapshottree ($prefix, $root, $snapshots);
>  		    }],
>  
>      rollback => [ "PVE::API2::Qemu", 'rollback', ['vmid', 'snapname'], { node => $nodename } , $upid_exit ],
>