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.eb...@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@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
