I haven't checked in detail whether patch #1 makes sense in isolation,
but this one here is not yet 100% there ;)
On January 7, 2020 4:55 pm, Oguz Bektas wrote:
> since we handle errors gracefully now, we don't need to write & save
> config every time we change a setting.
>
> note: this results in a change of behavior in the API. since errors are
> handled gracefully instead of "die"ing, when there is a pending change
> which cannot be applied for some reason, it will get logged in the
> tasklog but the vm will continue booting regardless. the non-applied
> change will stay in the pending section of the configuration.
>
> Signed-off-by: Oguz Bektas <o.bek...@proxmox.com>
> ---
>
> v3 -> v4:
> * add explanation in commit message about behavior change in API
>
> PVE/QemuServer.pm | 21 +++++----------------
> 1 file changed, 5 insertions(+), 16 deletions(-)
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 2de8376..9c86bee 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -4993,19 +4993,11 @@ sub vmconfig_apply_pending {
> my $force = $pending_delete_hash->{$opt}->{force};
> eval {
> die "internal error" if $opt =~ m/^unused/;
it would be nice to not mix if styles like this, it makes the flow
harder to read IMHO..
> - $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);
> } 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 = $@) {
> @@ -5013,24 +5005,20 @@ 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
this load_config here now throws away all the deletions we did in the
loop above!
>
> 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};
> }
the if and elsif could be combined into a single conditional..
my $old = $conf->{opt};
my $new = $conf->{pending}->{$opt};
if (defined($old) && is_valid_drivename($opt) && $old ne $new) {
vmconfig....
}
which IMHO is way easier to parse :)
> };
> if (my $err = $@) {
> @@ -5039,9 +5027,10 @@ sub vmconfig_apply_pending {
> $conf->{$opt} = delete $conf->{pending}->{$opt};
> PVE::QemuConfig->cleanup_pending($conf);
this does not make sense here, it should happen once between the two
loops maybe, but not in each iteration (cleanup_pending itself iterates
over the pending hash and the delete hash!)? if we call cleanup_pending
before the loop over pending, we could even drop the '$old ne $new' from
my proposed conditional, since that is guaranteed then ;)
> }
> -
> - 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
