Re: [pve-devel] [PATCH pve-manager 1/3] fix #3903: jobs-plugin: add remove vmid from jobs helper

[Fabian Ebner <f.ebner@proxmox.com>]

Am 01.03.22 um 09:51 schrieb Hannes Laimer:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  PVE/Jobs/Plugin.pm | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/Jobs/Plugin.pm b/PVE/Jobs/Plugin.pm
> index 6098360b..4883a193 100644
> --- a/PVE/Jobs/Plugin.pm
> +++ b/PVE/Jobs/Plugin.pm
> @@ -3,7 +3,7 @@ package PVE::Jobs::Plugin;
>  use strict;
>  use warnings;
>  
> -use PVE::Cluster qw(cfs_register_file);
> +use PVE::Cluster qw(cfs_register_file cfs_lock_file cfs_read_file cfs_write_file);
>  
>  use base qw(PVE::SectionConfig);
>  
> @@ -92,6 +92,23 @@ sub write_config {
>      $class->SUPER::write_config($filename, $cfg);
>  }
>  
> +sub remove_vmid_from_jobs {

Since this is iterating over jobs of all kinds and not something that
should be overridden by derived plugins, it should rather be part of
Jobs.pm.

If, in the future, plugins ever need more fine-grained control, we can
always add a helper method to remove a vmid from a single job and adapt
this function to call the helper method from the appropriate plugin for
each job.

> +    my ($vmid) = @_;

Style nit: we usually put a blank line after the parameter assignment.

> +    cfs_lock_file('jobs.cfg', undef, sub {
> +	my $jobs_data = cfs_read_file('jobs.cfg');
> +	while((my $id, my $job) = each (%{$jobs_data->{ids}})){


Style nits:
* We don't really use 'each'. One reason is that the state of the
iterator is not reset when returning early. Not relevant here, but it's
just not nice behavior.
* Missing spaces before/after outermost parentheses

> +	    next if !defined($job->{vmid});
> +	    $job->{vmid} = join(',', grep { $_ ne $vmid } PVE::Tools::split_list($job->{vmid}));
> +	    if ($job->{vmid} eq '') {
> +		delete $jobs_data->{ids}->{$id};
> +	    } else {
> +		$jobs_data->{ids}->{$id} = $job;

Isn't this just assigning the same reference again?

> +	    }
> +	}
> +	cfs_write_file('jobs.cfg', $jobs_data);
> +    });
> +}
> +
>  sub run {
>      my ($class, $cfg) = @_;
>      # implement in subclass