[pve-devel] [PATCH v5 qemu-server] vmconfig_apply_pending: remove redundant write/load config calls

[Fabian Grünbichler]

On January 14, 2020 5:36 pm, Oguz Bektas wrote:
> since we handle errors gracefully now, we don't need to write & save
> config every time we change a setting.
> 
> Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> ---
> v4 -> v5:
> 
> changed some stuff according to the feedback from fabian and thomas,
> thanks a lot!
> 
> * remove forgotten load_config call
> * some style changes (if block vs. postfix if) for consistency
> * do not run cleanup_pending in every iteration, but instead between the
> delete and add/change loops (for optimization)
> * remove unnecessary remove_from_pending_delete in the eval, since
> that's already handled afterwards in the else block
> 
>  PVE/QemuServer.pm | 41 ++++++++++++++---------------------------
>  1 file changed, 14 insertions(+), 27 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index f7d99e3..eb4ef85 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5108,20 +5108,13 @@ sub vmconfig_apply_pending {
>      foreach my $opt (sort keys %$pending_delete_hash) {
>  	my $force = $pending_delete_hash->{$opt}->{force};
>  	eval {
> -	    die "internal error" if $opt =~ m/^unused/;
> -	    $conf = PVE::QemuConfig->load_config($vmid); # update/reload
> -	    if (!defined($conf->{$opt})) {
> -		PVE::QemuConfig->remove_from_pending_delete($conf, $opt);
> -		PVE::QemuConfig->write_config($vmid, $conf);
> +	    if ($opt =~ m/^unused/) {
> +		die "internal error";
> +	    }
> +	    elsif (!defined($conf->{$opt})) {
> +		# pass

1.) style
2.) the if and elsif statements can be combined:

elsif (defined($conf->{$opt}) && is_valid_drivename($opt)) {
  vmconfig...
}

>  	    } elsif (is_valid_drivename($opt)) {
>  		vmconfig_delete_or_detach_drive($vmid, $storecfg, $conf, $opt, $force);
> -		PVE::QemuConfig->remove_from_pending_delete($conf, $opt);
> -		delete $conf->{$opt};
> -		PVE::QemuConfig->write_config($vmid, $conf);
> -	    } else {
> -		PVE::QemuConfig->remove_from_pending_delete($conf, $opt);
> -		delete $conf->{$opt};
> -		PVE::QemuConfig->write_config($vmid, $conf);
>  	    }
>  	};
>  	if (my $err = $@) {
> @@ -5129,35 +5122,29 @@ sub vmconfig_apply_pending {
>  	} else {
>  	    PVE::QemuConfig->remove_from_pending_delete($conf, $opt);
>  	    delete $conf->{$opt};
> -	    PVE::QemuConfig->write_config($vmid, $conf);
>  	}
>      }
>  
> -    $conf = PVE::QemuConfig->load_config($vmid); # update/reload
> +    PVE::QemuConfig->cleanup_pending($conf);

this ensures that no matching pending and current config can exist

>  
>      foreach my $opt (keys %{$conf->{pending}}) { # add/change
> -	$conf = PVE::QemuConfig->load_config($vmid); # update/reload
> -
> +	next if $opt eq 'delete'; # just to be sure
>  	eval {
> -	    if (defined($conf->{$opt}) && ($conf->{$opt} eq $conf->{pending}->{$opt})) {
> -		# skip if nothing changed
> -	    } elsif (is_valid_drivename($opt)) {
> -		vmconfig_register_unused_drive($storecfg, $vmid, $conf, parse_drive($opt, $conf->{$opt}))
> -		    if defined($conf->{$opt});
> -		$conf->{$opt} = $conf->{pending}->{$opt};
> -	    } else {
> -		$conf->{$opt} = $conf->{pending}->{$opt};
> +	    my $old_val = $conf->{$opt};
> +	    my $new_val = $conf->{pending}->{$opt};
> +	    if (defined($old_val) && is_valid_drivename($opt) && $old_val ne $new_val) {

so like I said, we don't actually need $new_val and the check here at 
all, since $old_val is always different from $new_val, so we can just do

if (defined($conf->{$opt}) && is_valid_drivename($opt)) {

which is also the exact same condition that we have in the other loop, 
which is nice ;)

> +		vmconfig_register_unused_drive($storecfg, $vmid, $conf, parse_drive($opt, $old_val))
>  	    }
>  	};
>  	if (my $err = $@) {
>  	    $add_apply_error->($opt, $err);
>  	} else {
>  	    $conf->{$opt} = delete $conf->{pending}->{$opt};
> -	    PVE::QemuConfig->cleanup_pending($conf);
>  	}
> -
> -	PVE::QemuConfig->write_config($vmid, $conf);
>      }
> +
> +    # write all changes at once to avoid unnecessary i/o
> +    PVE::QemuConfig->write_config($vmid, $conf);
>  }
>  
>  my $safe_num_ne = sub {
> -- 
> 2.20.1
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>