[pve-devel] [PATCH v5 manager 1/2] vzdump: move remaining guest include logic to single method

[Thomas Lamprecht]

Am 6/15/20 um 4:02 PM schrieb Aaron Lauterer:
> 
> 
> On 6/5/20 8:39 PM, Thomas Lamprecht wrote:
>> On 5/6/20 11:57 AM, Aaron Lauterer wrote:
> [...]
>>> @@ -1169,34 +1158,39 @@ sub get_included_guests {
>>>         my $nodename = PVE::INotify::nodename();
>>>       my $vmids = [];
>>> +    my $vmids_per_node = {};
>>> +
>>> +    my $vmlist = PVE::Cluster::get_vmlist();
>>>   -    # convert string lists to arrays
>>>       if ($job->{pool}) {
>>>       $vmids = PVE::API2Tools::get_resource_pool_guest_members($job->{pool});
>>> +    } elsif ($job->{vmid}) {
>>> +    $vmids = [ PVE::Tools::split_list($job->{vmid}) ];
>>>       } else {
>>> -    $vmids = [ PVE::Tools::split_list(extract_param($job, 'vmid')) ];
>>> +    # all or exclude
>>> +    my @exclude = PVE::Tools::split_list($job->{exclude});
>>> +    @exclude = @{PVE::VZDump::check_vmids(@exclude)};
>>> +    my $excludehash = { map { $_ => 1 } @exclude };
>>> +
>>> +    for my $id (keys %{ $vmlist->{ids} }) {
>>
>> you could sort it here already, avoiding the sort after the loop.
>>
>> Whole thing could also be written as:
>>
>> $vmids = [ grep { !$excludehash->{$_} } sort keys $vmlist->{ids}->%* ];
>>
>> but no hard feeling here.
>>
>>> +        next if $excludehash->{$id};
>>> +        push @$vmids, $id;
>>> +    }
>>>       }
>>> +    $vmids = [ sort {$a <=> $b} @$vmids];
>>>
> 
> I just realized that there was a reason to sort the vmids at this point. When doing the sorting here we sort them no matter the source. When selecting the VMs manually, the order in which they are selected in the GUI is the order in which they will be stored in the vzdump.cron file.
> 
> If we move the sorting into that for loop we will only sort them when we have an `all` and/or `exclude` job.

True, the other case could have been sorted when expanded into the vmids array directly
but one single sorting place /is/ nicer - it's not like this is performance critical
after all.