On 1/22/20 5:37 PM, Oguz Bektas wrote:
> this patch adds the partial_fast_plug function, which allows to partially
> fastplug an option with a property string.
>
> this is done by having a map $partial_fast_plug_option, the format is
> commented.
>
> other helper functions:
>
> * safe_boolean_ne (!$a != !$b)
> * typesafe_ne (combines safe_string_ne, safe_num_ne and safe_boolean_ne by
> taking $type as an argument)
>
> the qemu-guest-agent is our first use case for this (although i am sure
> there are more, this is more of a proof of concept. it should be trivial
> to add other things via the map), specifically the fstrim_cloned_disks
> option.
>
> Co-Authored-by: Stefan Reiter <s.rei...@proxmox.com>
> Signed-off-by: Oguz Bektas <o.bek...@proxmox.com>
> ---
>
> i added stefan as a co-author for this, to thank for his help debugging and
> testing it with me
no thanks for me providing the initial hunk of code doing (almost) all
of this ? ;P
>
> also please note that i'm sending this as RFC for review only, and it
> shouldn't be applied yet since i'm working on generalizing it a bit more
> for code reuse via pve-guest-common
As you only call that method in the agent case this is also not ready to
be taken in here. Some other issues below.
Testing changing the agent from a deactivated to a "change every setting" leads
to an error with your series:
> agent: hotplug problem - format error enabled: property is missing and it is
> not optional
That happens because $old is empty in that case but you try to parse it as
format
nonetheless.
>
> PVE/QemuServer.pm | 78 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 78 insertions(+)
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index bcdadca..74cbba0 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5031,6 +5031,10 @@ sub vmconfig_hotplug_pending {
> }
> vmconfig_update_disk($storecfg, $conf,
> $hotplug_features->{disk},
> $vmid, $opt, $value, 1, $arch,
> $machine_type);
> + } elsif ($opt eq 'agent') {
> + # partially fastpluggable
useless comment
> + # skip if all options were fastpluggable
No! You do the contrary, you skip if *not* all options were fastpluggable!
You could *always* skip if you deleted $cond->{pending}->{$opt} in
partial_fast_plug
if $no_changes there is false. If you want to do it like this and let the
deletion
from pending happen here, OK, but get the comments right - else this is
confusing
as hell.
> + die "skip\n" if partial_fast_plug($conf, $opt);
> } elsif ($opt =~ m/^memory$/) { #dimms
> die "skip\n" if !$hotplug_features->{memory};
> $value = PVE::QemuServer::Memory::qemu_memory_hotplug($vmid,
> $conf, $defaults, $opt, $value);
> @@ -5165,6 +5169,80 @@ my $safe_string_ne = sub {
> return $a ne $b;
> };
>
> +my $safe_boolean_ne = sub {
> + my ($a, $b) = @_;
> +
> + # we don't check if value is defined, since undefined
> + # is false (so it's a valid boolean)
The comment below is enough, above adds just noise - it's a common enough
pattern, IMO.
> + # negate both values to normalize and compare
> + return !$a != !$b;
> +};
> +
> +my $typesafe_ne = sub {
> + my ($a, $b, $type) = @_;
> +
> + return 0 if !defined($a) && !defined($b);
> + return 1 if !defined($a);
> + return 1 if !defined($b);
> +
> + if ($type eq 'string') {
> + $safe_string_ne->($a, $b);
1. The returns are still missing, as told off-list. missing those works by
chance only
and is very prone for subtle breakage.
2. if you move this to guest-common do the check directly here, no point in
calling (and
then also moving that) method for a single line. I.e., just do here:
return $a ne $b;
> + } elsif ($type eq 'number') {
> + $safe_num_ne->($a, $b);
return $a != $b;
> + } elsif ($type eq 'boolean') {
> + $safe_boolean_ne->($a, $b);
return !$a != !$b;
directly.
> + }
> +};
> +
> +my $partial_fast_plug_option =
> +# name
> +# -> fmt -> format variable
> +# -> properties -> fastpluggable options hash
move above variable def.
> +{
this belongs at the line of variable def
> + agent => {
> + fmt => $agent_fmt,
Maybe we could/should really get the format from the $confdesc and simplify this
to
my $partial_fast_plug_option = {
agent => {
fstrim_cloned_disks => 1
},
};
> + properties => {
> + fstrim_cloned_disks => 1
> + },
> + },
> +};
> +
> +sub partial_fast_plug {
> + my ($conf, $opt) = @_;
> +
> + my $format = $partial_fast_plug_option->{$opt}->{fmt};
> + my $properties = $partial_fast_plug_option->{$opt}->{properties};
if you would call this $fast_pluggable instead of this over general name
you could avoid that extra $is_fastpluggable variable below.
> +
> + my $old = PVE::JSONSchema::parse_property_string($format, $conf->{$opt});
my $old = {};
if (exists($conf->{$opt})) {
$old = PVE::JSONSchema::parse_property_string($format, $conf->{$opt});
}
Else you throw up if $conf->{$opt} was not set previously.
> + my $new = PVE::JSONSchema::parse_property_string($format,
> $conf->{pending}->{$opt});
above should be OK as the pending $opt has to be always set, else this mustn't
be called.
> +
> + my $changes_left = 0;
> +
> + # merge old and new opts to iterate
> + my $all_opts = dclone($old);
> + foreach my $opt1 (keys %$new) {
> + $all_opts->{$opt1} = $new->{$opt1} if !defined($all_opts->{$opt1});
> + }
Instead of above 5 lines you could just do:
my @all_keys = keys %{{ %$new, %$old }};
(you only need the keys, not the data)
> +
> + foreach my $opt2 (keys %$all_opts) {
you can reuse variable names from for(each) in different scopes, no need
to use $opt1, $opt2, ...
> + my $is_fastpluggable = $properties->{$opt2};
> + my $type = $format->{$opt2}->{type};
> + if ($typesafe_ne->($old->{$opt2}, $new->{$opt2}, $type)) {
> + if (defined($is_fastpluggable)) {
why defined check? We control that definition, so normal "boolean" check is
enough,
with above suggested changes together just write:
if ($fast_pluggable->{$opt}) {
...
> + $old->{$opt2} = $new->{$opt2};
> + } else {
> + $changes_left = 1;
> + }
> + }
> + }
> +
> + $conf->{$opt} = PVE::JSONSchema::print_property_string($old, $format);
above needs to be guarded too, else this can throw up too:
if (keys %$old) {
$conf->{$opt} = PVE::JSONSchema::print_property_string($old, $format);
}
> +
> + return $changes_left;
> +}
> +
> +
> sub vmconfig_update_net {
> my ($storecfg, $conf, $hotplug, $vmid, $opt, $value, $arch,
> $machine_type) = @_;
>
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
