[pve-devel] [PATCH guest-common 1/2] move pending changes related functions into AbstractConfig

[Fabian Grünbichler]

meta: please send v2 together with v2 of pve-container (and future) patches.

On September 4, 2019 6:00 pm, Oguz Bektas wrote:
> some functions from Qemu w.r.t. pending changes can be moved to
> AbstractConfig, in order to make them available for both QemuConfig and
> LXC::Config.
> 
> Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> ---
>  PVE/AbstractConfig.pm | 79 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/PVE/AbstractConfig.pm b/PVE/AbstractConfig.pm
> index e0d0f10..18522b9 100644
> --- a/PVE/AbstractConfig.pm
> +++ b/PVE/AbstractConfig.pm
> @@ -68,6 +68,85 @@ sub write_config {
>      PVE::Cluster::cfs_write_file($cfspath, $conf);
>  }
>  
> +## Pending changes related
> +
> +sub split_flagged_list {
> +    my ($class, $text) = @_;
> +    $text ||= '';
> +    $text =~ s/[,;]/ /g;
> +    $text =~ s/^\s+//;
> +    return { map { /^(!?)(.*)$/ && ($2, $1) } ($text =~ /\S+/g) };
> +}
> +
> +sub join_flagged_list {
> +    my ($class, $how, $lst) = @_;
> +    join $how, map { $lst->{$_} . $_ } keys %$lst;
> +}

these two are really only used for the pending delete hash, so instead 
of just moving them with this strange naming scheme, why not simplify 
and rename them to

parse_pending_delete($class, $raw)
print_pending_delete($class, $pending_delete_hash)

we need versioned breaks/depends anyway.. while we are at it, instead of 
$hash->{foo} being either '!' or '' (which is just how the $force 
property is encoded in the 'flagged list', but not a good representation 
in a perl hash), it would be nicer to have
$hash->{foo}->{force} being 0/1 IMHO.

> +
> +sub vmconfig_delete_pending_option {
> +    my ($class, $conf, $key, $force) = @_;
> +
> +    delete $conf->{pending}->{$key};
> +    my $pending_delete_hash = $class->split_flagged_list($conf->{pending}->{delete});
> +    $pending_delete_hash->{$key} = $force ? '!' : '';
> +    $conf->{pending}->{delete} = $class->join_flagged_list(',', $pending_delete_hash);
> +}
> +
> +sub vmconfig_undelete_pending_option {
> +    my ($class, $conf, $key) = @_;
> +
> +    my $pending_delete_hash = $class->split_flagged_list($conf->{pending}->{delete});
> +    delete $pending_delete_hash->{$key};
> +
> +    if (%$pending_delete_hash) {
> +	$conf->{pending}->{delete} = $class->join_flagged_list(',', $pending_delete_hash);
> +    } else {
> +	delete $conf->{pending}->{delete};
> +    }
> +}

ugh, those two are also not very well named. not your fault, but IMHO 
also an opportunity to think about a better name if we refactor this. 
there are only 9 existing callers after all..

they don't delete or undelete a pending option (whatever that would 
actually do ;)), they queue an option for deletion via the pending 
delete hash/list..

add_to_pending_delete
remove_from_pending_delete

? better proposals welcome, like always ;)

> +
> +sub vmconfig_cleanup_pending {

I'd drop the 'vmconfig_' from the name here

> +    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->split_flagged_list($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->join_flagged_list(',', $pending_delete_hash);
> +    } else {
> +	delete $conf->{pending}->{delete};
> +    }
> +
> +    return $changes;
> +}
> +
> +sub vmconfig_hotplug_pending {

same - although I am not sure whether I'd not opt for this sub to stay 
in QemuServer.pm altogether

the interdependency between QemuServer and QemuConfig is already way too 
messy for my taste, and moving this more than doubles the calls from 
QemuConfig to QemuServer:

old:
QemuServer->QemuConfig: 70 (these are fine!)
QemuConfig->QemuServer: 29 (these are only there because we did not want 
to move 60% of QemuServer.pm including the $confdesc to QemuConfig)

new:
QemuServer->QemuConfig: 59 (-11)
QemuConfig->QemuServer: 69 (+40)

the -11 are actually all load/write_config..

moving this sub also adds another module dependency cycle:

QemuServer->QemuConfig->QemuServer::CloudInit->QemuServer

> +    my ($class, $vmid, $conf, $storecfg, $selection, $errors) = @_;
> +    die "implement me - abstract method\n";
> +}
> +
> +sub vmconfig_apply_pending {

same (name if moved, but maybe not move at all?)

> +    my ($class, $vmid, $conf, $storecfg) = @_;
> +    die "implement me - abstract method\n";
> +}
> +
> +
>  # 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
> 
>