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