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.bek...@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@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
