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