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

[Fabian Grünbichler]

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)

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

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