[pve-devel] [RFC qemu-server] qemu: allow partial fastplugging of property string options

[Thomas Lamprecht]

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.reiter at proxmox.com>
> Signed-off-by: Oguz Bektas <o.bektas at 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) = @_;
>  
>