* [pve-devel] [PATCH qemu-server] apply pending changes at vm stop only if the section is not empty
@ 2021-12-01 12:00 Oguz Bektas
2021-12-01 14:10 ` Fabian Ebner
0 siblings, 1 reply; 3+ messages in thread
From: Oguz Bektas @ 2021-12-01 12:00 UTC (permalink / raw)
To: pve-devel
we also have the same skipping logic when starting vms. this way we
avoid calling 'write_config' when there are no present changes to
commit.
Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
PVE/QemuServer.pm | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 45b704d..1379f33 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -4998,6 +4998,8 @@ sub vmconfig_delete_or_detach_drive {
sub vmconfig_apply_pending {
my ($vmid, $conf, $storecfg, $errors) = @_;
+ return if !scalar(keys %{$conf->{pending}});
+
my $add_apply_error = sub {
my ($opt, $msg) = @_;
my $err_msg = "unable to apply pending change $opt : $msg";
@@ -5889,7 +5891,7 @@ sub _do_vm_stop {
die "VM quit/powerdown failed - got timeout\n";
}
} else {
- vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1) if $conf;
+ vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, scalar(keys %{$conf->{pending}})) if $conf;
return;
}
} else {
@@ -5920,7 +5922,7 @@ sub _do_vm_stop {
sleep 1;
}
- vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1) if $conf;
+ vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, scalar(keys %{$conf->{pending}})) if $conf;
}
# Note: use $nocheck to skip tests if VM configuration file exists.
--
2.30.2
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH qemu-server] apply pending changes at vm stop only if the section is not empty
2021-12-01 12:00 [pve-devel] [PATCH qemu-server] apply pending changes at vm stop only if the section is not empty Oguz Bektas
@ 2021-12-01 14:10 ` Fabian Ebner
2021-12-01 15:10 ` Oguz Bektas
0 siblings, 1 reply; 3+ messages in thread
From: Fabian Ebner @ 2021-12-01 14:10 UTC (permalink / raw)
To: pve-devel, Oguz Bektas
Am 01.12.21 um 13:00 schrieb Oguz Bektas:
> we also have the same skipping logic when starting vms. this way we
> avoid calling 'write_config' when there are no present changes to
> commit.
>
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
> PVE/QemuServer.pm | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 45b704d..1379f33 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -4998,6 +4998,8 @@ sub vmconfig_delete_or_detach_drive {
> sub vmconfig_apply_pending {
> my ($vmid, $conf, $storecfg, $errors) = @_;
>
> + return if !scalar(keys %{$conf->{pending}});
Isn't this part of the patch enough? For the caller while starting a VM
you mentioned, there is an additional load_config() that is skipped if
there are no pending changes, but for the two callers below, there is no
such benefit. They just indicate that any pending changes should be
applied, which is fine even if there are none.
> +
> my $add_apply_error = sub {
> my ($opt, $msg) = @_;
> my $err_msg = "unable to apply pending change $opt : $msg";
> @@ -5889,7 +5891,7 @@ sub _do_vm_stop {
> die "VM quit/powerdown failed - got timeout\n";
> }
> } else {
> - vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1) if $conf;
> + vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, scalar(keys %{$conf->{pending}})) if $conf;
Style nit: >100 characters
> return;
> }
> } else {
> @@ -5920,7 +5922,7 @@ sub _do_vm_stop {
> sleep 1;
> }
>
> - vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1) if $conf;
> + vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, scalar(keys %{$conf->{pending}})) if $conf;
> }
>
> # Note: use $nocheck to skip tests if VM configuration file exists.
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH qemu-server] apply pending changes at vm stop only if the section is not empty
2021-12-01 14:10 ` Fabian Ebner
@ 2021-12-01 15:10 ` Oguz Bektas
0 siblings, 0 replies; 3+ messages in thread
From: Oguz Bektas @ 2021-12-01 15:10 UTC (permalink / raw)
To: Fabian Ebner; +Cc: pve-devel
On Wed, Dec 01, 2021 at 03:10:28PM +0100, Fabian Ebner wrote:
> Am 01.12.21 um 13:00 schrieb Oguz Bektas:
> > we also have the same skipping logic when starting vms. this way we
> > avoid calling 'write_config' when there are no present changes to
> > commit.
> >
> > Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> > ---
> > PVE/QemuServer.pm | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> > index 45b704d..1379f33 100644
> > --- a/PVE/QemuServer.pm
> > +++ b/PVE/QemuServer.pm
> > @@ -4998,6 +4998,8 @@ sub vmconfig_delete_or_detach_drive {
> > sub vmconfig_apply_pending {
> > my ($vmid, $conf, $storecfg, $errors) = @_;
> > + return if !scalar(keys %{$conf->{pending}});
>
> Isn't this part of the patch enough? For the caller while starting a VM you
> mentioned, there is an additional load_config() that is skipped if there are
> no pending changes, but for the two callers below, there is no such benefit.
> They just indicate that any pending changes should be applied, which is fine
> even if there are none.
yes i guess it's alright to keep this part only :) though we could still
avoid the vmconfig_apply_pending call in vm_stop_cleanup completely if
the section is empty anyway. not that it brings much but still it
should be fine..
>
> > +
> > my $add_apply_error = sub {
> > my ($opt, $msg) = @_;
> > my $err_msg = "unable to apply pending change $opt : $msg";
> > @@ -5889,7 +5891,7 @@ sub _do_vm_stop {
> > die "VM quit/powerdown failed - got timeout\n";
> > }
> > } else {
> > - vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1) if $conf;
> > + vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, scalar(keys %{$conf->{pending}})) if $conf;
>
> Style nit: >100 characters
>
> > return;
> > }
> > } else {
> > @@ -5920,7 +5922,7 @@ sub _do_vm_stop {
> > sleep 1;
> > }
> > - vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1) if $conf;
> > + vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, scalar(keys %{$conf->{pending}})) if $conf;
> > }
> > # Note: use $nocheck to skip tests if VM configuration file exists.
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-12-01 15:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-01 12:00 [pve-devel] [PATCH qemu-server] apply pending changes at vm stop only if the section is not empty Oguz Bektas
2021-12-01 14:10 ` Fabian Ebner
2021-12-01 15:10 ` Oguz Bektas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox