[pve-devel] [PATCH v2 guest-common 01/18] refactor pending changes related config code into AbstractConfig

[Fabian Grünbichler]

On September 30, 2019 2:44 pm, Oguz Bektas wrote:
> also use a better naming scheme for methods:
> 
> split_flagged_list -> parse_pending_delete
> join_flagged_list -> print_pending_delete
> vmconfig_delete_pending_option -> add_to_pending_delete
> vmconfig_undelete_pending_option -> remove_from_pending_delete
> vmconfig_cleanup_pending -> cleanup_pending
> 
> Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> ---
>  PVE/AbstractConfig.pm | 68 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/PVE/AbstractConfig.pm b/PVE/AbstractConfig.pm
> index e0d0f10..910ca86 100644
> --- a/PVE/AbstractConfig.pm
> +++ b/PVE/AbstractConfig.pm
> @@ -68,6 +68,74 @@ sub write_config {
>      PVE::Cluster::cfs_write_file($cfspath, $conf);
>  }
>  
> +# Pending changes related
> +
> +sub parse_pending_delete {
> +    my ($class, $text) = @_;

conventionally, $text is called $data in our parse_ subs

> +    $text ||= '';
> +    $text =~ s/[,;]/ /g;
> +    $text =~ s/^\s+//;
> +    return { map { /^(!?)(.*)$/ && ($2, $1) } ($text =~ /\S+/g) };

like I said in v1 of this part, why not use the opportunity to give this 
a sane perl representation?

e.g.,

{
  'opt1' => { 'force' => 0 },
  'opt2' => { 'force' => 1 },
  'opt3' => { 'force' => 0 },
}

then you could still iterate over all pending deletions, and where you 
need that info, you can check if $pending_delete_hash->{$opt}->{force}?

right now, if you accidentally do

if ($pending_delete_hash->{$opt}) 

instead of

if (defined($pending_delete_hash->{$opt}))
or
if (exists($pending_delete_hash->{$opt}))

you probably do the opposite of what you want ;) there is only a single 
such caller atm, and that one is correct, but it's very easy to get 
wrong IMHO.

> +}
> +
> +sub print_pending_delete {
> +    my ($class, $how, $lst) = @_;
> +    join $how, map { $lst->{$_} . $_ } keys %$lst;

we always want to join with ',', so why make it configurable? also, this 
ain't no l(i)st anymore ;) if we change the $pending_delete_hash 
structure, this obviously needs to be adapted to encode the 
'forcefulness' correctly.

> +}
> +
> +sub add_to_pending_delete {
> +    my ($class, $conf, $key, $force) = @_;
> +
> +    delete $conf->{pending}->{$key};
> +    my $pending_delete_hash = $class->parse_pending_delete($conf->{pending}->{delete});
> +    $pending_delete_hash->{$key} = $force ? '!' : '';
> +    $conf->{pending}->{delete} = $class->print_pending_delete(',', $pending_delete_hash);
> +}

also would need adaption if we don't store '!' in the hash.

> +
> +sub remove_from_pending_delete {
> +    my ($class, $conf, $key) = @_;
> +
> +    my $pending_delete_hash = $class->parse_pending_delete($conf->{pending}->{delete});
> +    delete $pending_delete_hash->{$key};
> +
> +    if (%$pending_delete_hash) {
> +	$conf->{pending}->{delete} = $class->print_pending_delete(',', $pending_delete_hash);
> +    } else {
> +	delete $conf->{pending}->{delete};
> +    }
> +}
> +
> +sub cleanup_pending {
> +    my ($class, $conf) = @_;
> +
> +    # remove pending changes when nothing changed
> +    my $changes;
> +    foreach my $opt (keys %{$conf->{pending}}) {
> +	if (defined($conf->{$opt}) && ($conf->{pending}->{$opt} eq  $conf->{$opt})) {
> +	    $changes = 1;
> +	    delete $conf->{pending}->{$opt};
> +	}
> +    }
> +
> +    my $current_delete_hash = $class->parse_pending_delete($conf->{pending}->{delete});
> +    my $pending_delete_hash = {};
> +    while (my ($opt, $force) = each %$current_delete_hash) {
> +	if (defined($conf->{$opt})) {
> +	    $pending_delete_hash->{$opt} = $force;
> +	} else {
> +	    $changes = 1;
> +	}
> +    }
> +
> +    if (%$pending_delete_hash) {
> +	$conf->{pending}->{delete} = $class->print_pending_delete(',', $pending_delete_hash);
> +    } else {
> +	delete $conf->{pending}->{delete};
> +    }
> +
> +    return $changes;
> +}
> +
>  # Lock config file using flock, run $code with @param, unlock config file.
>  # $timeout is the maximum time to aquire the flock
>  sub lock_config_full {
> -- 
> 2.20.1
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>