[pve-devel] [PATCH guest-common 4/5] abstractconfig: add partial_fast_plug

[Stefan Reiter]

On 1/28/20 4:03 PM, Oguz Bektas wrote:
> allows partial fast plugging of functions as defined in the
> $partial_fast_plug_option in qemuserver (and possibly lxc later on)
> 
> Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> ---
>   PVE/AbstractConfig.pm | 44 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 44 insertions(+)
> 
> diff --git a/PVE/AbstractConfig.pm b/PVE/AbstractConfig.pm
> index b63a744..102b12d 100644
> --- a/PVE/AbstractConfig.pm
> +++ b/PVE/AbstractConfig.pm
> @@ -168,6 +168,50 @@ sub cleanup_pending {
>       return $changes;
>   }
>   
> +sub get_partial_fast_plug_map {
> +    my ($class) = @_;
> +
> +    die "abstract method - implement me ";
> +}
> +
> +sub partial_fast_plug {
> +    my ($class, $conf, $opt) = @_;
> +
> +    my $partial_fast_plug_option = $class->get_partial_fast_plug_map();

see 5/5 on naming

> +    my $format = $partial_fast_plug_option->{$opt}->{fmt};
> +    my $fast_pluggable = $partial_fast_plug_option->{$opt}->{properties};
> +
> +    my $old = {};
> +    if (exists($conf->{$opt})) {
> +	$old = PVE::JSONSchema::parse_property_string($format, $conf->{$opt});
> +    }
> +    my $new = PVE::JSONSchema::parse_property_string($format, $conf->{pending}->{$opt});

Not a fan of $old/$new naming here, especially since $old is actually 
the "new" config that you set after fast-plugging as well. How about 
s/old/configured/ and s/new/pending/ ?

> +
> +    my $changes_left = 0;
> +
> +    # merge old and new opts to iterate
> +    my @all_keys = keys %{{ %$new, %$old }};
> +
> +    foreach my $subopt (@all_keys) {
> +	my $type = $format->{$subopt}->{type};
> +	if (PVE::GuestHelpers::typesafe_ne($old->{$subopt}, $new->{$subopt}, $type)) {
> +	    if ($fast_pluggable->{$subopt}) {
> +		$old->{$subopt} = $new->{$subopt};
> +	    } else {
> +		$changes_left = 1;
> +	    }
> +	}
> +    }
> +
> +    # fastplug

I'm the last person to call you out for useless comments, but this one 
could be fleshed out a bit or removed I think ;)

(Maybe note why it's okay to not set $conf->{$opt} if $old is empty)

> +    if (keys %$old) {
> +	$conf->{$opt} = PVE::JSONSchema::print_property_string($old, $format);
> +    }
> +

Missing a check for $changes_left here: If it's 0, you can remove $opt 
from $conf->{pending}, since otherwise the exact same config that is now 
set will also be in pending. Doesn't hurt, but if you have the info 
anyway it'd be easy to fix and makes it a bit cleaner.

> +    return $changes_left;

see 5/5 on returned value here

> +}
> +
> +
>   sub load_snapshot_config {
>       my ($class, $vmid, $snapname) = @_;
>   
>