[pve-devel] [PATCH v3 guest-common 03/22] foreach_volume: generalize and implement function

[Fabian Ebner]

On 16.03.20 12:05, Fabian Grünbichler wrote:
> On March 12, 2020 1:08 pm, Fabian Ebner wrote:
>> Introduce a parameter $opts to allow for better control of which
>> keys/volumes to use for the iteration and ability to reverse the order.
>> Also, allow extra parameters for the function.
>>
>> Removes the '__snapshot'-prefix for future use from outside the module.
>>
>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>> ---
>>   PVE/AbstractConfig.pm | 40 +++++++++++++++++++++++++++++-----------
>>   1 file changed, 29 insertions(+), 11 deletions(-)
>>
>> diff --git a/PVE/AbstractConfig.pm b/PVE/AbstractConfig.pm
>> index 5b1683b..f2e130c 100644
>> --- a/PVE/AbstractConfig.pm
>> +++ b/PVE/AbstractConfig.pm
>> @@ -432,6 +432,31 @@ sub foreach_unused_volume {
>>       }
>>   }
>>   
>> +# Iterate over all configured volumes, calling $func for each key/value pair
>> +# with additional parameters @param.
>> +# By default, unused volumes and specials like vmstate are excluded.
>> +# Options: reverse         - reverses the order for the iteration
>> +#	   include_unused  - also iterate over unused volumes
>> +#	   extra_keys      - an array of extra keys to use for the iteration
>> +sub foreach_volume {
>> +    my ($class, $conf, $opts, $func, @param) = @_;
> 
> would it make sense to have foreach_volume with empty $opts as a wrapper
> around foreach_volume_full which is your foreach_volume? I expect many
> calls will just use the default/empty options..
> 
> it would allow us to drop foreach_mountpoint as wrapper around
> foreach_volume with empty config as well. all but one(!) call in
> pve-container are plain foreach_mountpoint..
> 
> many calls in qemu-server now use PVE::QemuServer::Drive::foreach_drive,
> but could actually use PVE::QemuConfig->foreach_volume (in fact, I think
> if we move back foreach_volid to QemuServer.pm we could even drop
> foreach_drive altogether? as always, hindsight is 20/20 ;))
> 
> I'd really like to avoid adding an abstract, general iterator over
> volumes for pve-container and qemu-server, but then only using it for
> half of the call sites. it's okay to have some specialized helpers like
> foreach_volid if they are used more than once, but the end result should
> not be that we now have
> 
> - foreach_volume (abstract/general)
> - foreach_mountpoint(_reverse) (pve-container, as wrapper around
>    foreach-volume)
> - foreach_drive (qemu-server, which basically does the same as
>    foreach_volume, but in a file where we can't call foreach_volume)
> 

Ok, I didn't think about getting rid of foreach_drive altogether, but it 
sounds like the right approach.

>> +
>> +    my @keys = $class->valid_volume_keys($opts->{reverse});
>> +    push @keys, @{$opts->{extra_keys}} if $opts->{extra_keys};
> 
> I am not sure what the semantics with extra_keys and reverse should be
> (currently reverse is just used for mounting LXC mountpoints IIRC, where
> we probably never have any extra_keys and don't set unused volumes
> either.
> 
> but the whole purpose for the reverse option is to do
> 
> foreach_volume(..., do_something())
> 
> ...
> 
> foreach_volume(..., { reverse => 1 }, undo_something())
> 
> where ordering is important. so we should either
> 
> die "'reverse' iteration only supported for default keys\n"
>      if reverse && (extra_keys || include_unused)
> 
> or make sure that extra_keys and include_unused also follow reverse
> semantics.
> 

I feel like dying is the best approach here, as it keeps the function 
simple and can always be changed later if reversing with non-default 
keys is ever needed.

>> +
>> +    foreach my $key (@keys) {
>> +	my $volume_string = $conf->{$key};
>> +	next if !defined($volume_string);
>> +
>> +	my $volume = $class->parse_volume($key, $volume_string, 1);
>> +	next if !defined($volume);
>> +
>> +	$func->($key, $volume, @param);
>> +    }
>> +
>> +    $class->foreach_unused_volume($conf, $func, @param) if $opts->{include_unused};
>> +}
>> +
>>   # Returns whether the template parameter is set in $conf.
>>   sub is_template {
>>       my ($class, $conf) = @_;
>> @@ -583,13 +608,6 @@ sub __snapshot_rollback_get_unused {
>>       die "abstract method - implement me\n";
>>   }
>>   
>> -# Iterate over all configured volumes, calling $func for each key/value pair.
>> -sub __snapshot_foreach_volume {
>> -    my ($class, $conf, $func) = @_;
>> -
>> -    die "abstract method - implement me\n";
>> -}
>> -
>>   # Copy the current config $source to the snapshot config $dest
>>   sub __snapshot_copy_config {
>>       my ($class, $source, $dest) = @_;
>> @@ -722,7 +740,7 @@ sub snapshot_create {
>>   
>>   	$class->__snapshot_create_vol_snapshots_hook($vmid, $snap, $running, "before");
>>   
>> -	$class->__snapshot_foreach_volume($snap, sub {
>> +	$class->foreach_volume($snap, undef, sub {
>>   	    my ($vs, $volume) = @_;
>>   
>>   	    $class->__snapshot_create_vol_snapshot($vmid, $vs, $volume, $snapname);
>> @@ -814,7 +832,7 @@ sub snapshot_delete {
>>       };
>>   
>>       # now remove all volume snapshots
>> -    $class->__snapshot_foreach_volume($snap, sub {
>> +    $class->foreach_volume($snap, undef, sub {
>>   	my ($vs, $volume) = @_;
>>   
>>   	return if $snapname eq 'vzdump' && $vs ne 'rootfs' && !$volume->{backup};
>> @@ -888,7 +906,7 @@ sub snapshot_rollback {
>>       
>>       my $snap = &$get_snapshot_config();
>>   
>> -    $class->__snapshot_foreach_volume($snap, sub {
>> +    $class->foreach_volume($snap, undef, sub {
>>   	my ($vs, $volume) = @_;
>>   
>>   	$class->__snapshot_rollback_vol_possible($volume, $snapname);
>> @@ -941,7 +959,7 @@ sub snapshot_rollback {
>>   
>>       $class->lock_config($vmid, $updatefn);
>>   
>> -    $class->__snapshot_foreach_volume($snap, sub {
>> +    $class->foreach_volume($snap, undef, sub {
>>   	my ($vs, $volume) = @_;
>>   
>>   	$class->__snapshot_rollback_vol_rollback($volume, $snapname);
>> -- 
>> 2.20.1
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>