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

[Rhonda D'Vine]

   Hi,

 thanks for the suggestions, they helped a lot. :)

On 5/8/19 11:26 AM, Thomas Lamprecht wrote:
> On 5/3/19 11:48 AM, Rhonda D'Vine wrote:
>> @@ -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);

 Thanks, picked up.

> 
>> +			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).

 Right, isn't less readable neither. :)

>> +
>> +			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)

 Gotcha, still picked it up, makes it a bit easier to read too.

>> +			# sort the elements by snaptime - with "current" (no snaptime) highest
>> +			sub snaptimesort {
> 
> 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.

 I don't see how it can be false, unless you took a snapshot on
1970-01-01, but better be on the safe side indeed.

>> +			    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.

 That though doesn't work here, it is defined with an empty string.
Potentially that might be a bug where originally the data structure is
generated, because for the current running system the snaptime value
isn't defined, but that would need more digging.

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

 That part looks only like changed by me because shifting around, it was
there before that way.  Which doesn't mean that we couldn't clean it up,
just as an explanation why it's there that way. :)  And yes, I assume
that the intention is to only get the first line indeed.  That would
this though:

($description) = $description =~ m/(.*)$/m;

 New patch coming, as single one though this time.

 Thanks,
Rhonda

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.proxmox.com/pipermail/pve-devel/attachments/20190510/2db30bfd/attachment.sig>