[pve-devel] [PATCH qemu-server] vm_resume: remove the $nocheck parameter

[Thomas Lamprecht]

nocheck was introduced as a band-aid in
commit: 289e0b8564dce494481cd3a5d534b801835f85b6 , but it only
accomplishes skipping the check_running call in the vm_qmp_command
(which vm_mon_cmd uses) and avoiding the load_conf for some race
cases with migration where the config wasn't moved yet at this point.

But as neither cont nor system_wakup will work on an non-running VM
we could use only vm_mon_cmd once at the end of the locked scope

Further nocheck may be removed, the load_conf could be wrapped in
skiplock, which the migration case uses anyway.
The qm skiplock param would need to be kept as dummy param for now,
to keep incoming migration backward compatibility...

Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
---

as proposed in:
https://pve.proxmox.com/pipermail/pve-devel/2018-June/032418.html

 PVE/API2/Qemu.pm  | 19 +++++++++----------
 PVE/CLI/qm.pm     |  2 +-
 PVE/QemuServer.pm | 19 ++++++-------------
 3 files changed, 16 insertions(+), 24 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index c15c71f..0e4e8d2 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -301,11 +301,12 @@ my $cloudinitoptions = {
 };
 
 my $check_vm_modify_config_perm = sub {
-    my ($rpcenv, $authuser, $vmid, $pool, $key_list) = @_;
+    my ($rpcenv, $vmid, $pool, $params) = @_;
 
+    my $authuser = $rpcenv->get_user();
     return 1 if $authuser eq 'root at pam';
 
-    foreach my $opt (@$key_list) {
+    foreach my $opt (keys %$params) {
 	# disk checks need to be done somewhere else
 	next if PVE::QemuServer::is_valid_drivename($opt);
 	next if $opt eq 'cdrom';
@@ -516,7 +517,7 @@ __PACKAGE__->register_method({
 
 	    &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param, $storage);
 
-	    &$check_vm_modify_config_perm($rpcenv, $authuser, $vmid, $pool, [ keys %$param]);
+	    $check_vm_modify_config_perm->($rpcenv, $vmid, $pool, [ keys %$param]);
 
 	    foreach my $opt (keys %$param) {
 		if (PVE::QemuServer::is_valid_drivename($opt)) {
@@ -1094,9 +1095,9 @@ my $update_vm_api  = sub {
 	}
     }
 
-    &$check_vm_modify_config_perm($rpcenv, $authuser, $vmid, undef, [@delete]);
+    $check_vm_modify_config_perm->($rpcenv, $vmid, undef, [@delete]);
 
-    &$check_vm_modify_config_perm($rpcenv, $authuser, $vmid, undef, [keys %$param]);
+    $check_vm_modify_config_perm->($rpcenv, $vmid, undef, [keys %$param]);
 
     &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param);
 
@@ -2349,7 +2350,7 @@ __PACKAGE__->register_method({
 	    vmid => get_standard_option('pve-vmid',
 					{ completion => \&PVE::QemuServer::complete_vmid_running }),
 	    skiplock => get_standard_option('skiplock'),
-	    nocheck => { type => 'boolean', optional => 1 },
+	    nocheck => { type => 'boolean', optional => 1, description => 'deprecated' },
 
 	},
     },
@@ -2371,16 +2372,14 @@ __PACKAGE__->register_method({
 	raise_param_exc({ skiplock => "Only root may use this option." })
 	    if $skiplock && $authuser ne 'root at pam';
 
-	my $nocheck = extract_param($param, 'nocheck');
-
-	die "VM $vmid not running\n" if !PVE::QemuServer::check_running($vmid, $nocheck);
+	die "VM $vmid not running\n" if !PVE::QemuServer::check_running($vmid);
 
 	my $realcmd = sub {
 	    my $upid = shift;
 
 	    syslog('info', "resume VM $vmid: $upid\n");
 
-	    PVE::QemuServer::vm_resume($vmid, $skiplock, $nocheck);
+	    PVE::QemuServer::vm_resume($vmid, $skiplock);
 
 	    return;
 	};
diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index 48fbc5f..4a38d0c 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -296,7 +296,7 @@ __PACKAGE__->register_method ({
 	    } elsif ($line =~ /^resume (\d+)$/) {
 		my $vmid = $1;
 		if (PVE::QemuServer::check_running($vmid, 1)) {
-		    eval { PVE::QemuServer::vm_resume($vmid, 1, 1); };
+		    eval { PVE::QemuServer::vm_resume($vmid, 1) };
 		    if ($@) {
 			$tunnel_write->("ERR: resume failed - $@");
 		    } else {
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index d6efb3a..cfa561b 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5167,29 +5167,22 @@ sub vm_suspend {
 }
 
 sub vm_resume {
-    my ($vmid, $skiplock, $nocheck) = @_;
+    my ($vmid, $skiplock) = @_;
 
     PVE::QemuConfig->lock_config($vmid, sub {
 
 	my $res = vm_mon_cmd($vmid, 'query-status');
 	my $resume_cmd = 'cont';
-
 	if ($res->{status} && $res->{status} eq 'suspended') {
 	    $resume_cmd = 'system_wakeup';
 	}
 
-	if (!$nocheck) {
-
+	if (!$skiplock) {
 	    my $conf = PVE::QemuConfig->load_config($vmid);
-
-	    PVE::QemuConfig->check_lock($conf)
-		if !($skiplock || PVE::QemuConfig->has_lock($conf, 'backup'));
-
-	    vm_mon_cmd($vmid, $resume_cmd);
-
-	} else {
-	    vm_mon_cmd_nocheck($vmid, $resume_cmd);
+	    PVE::QemuConfig->check_lock($conf) if !PVE::QemuConfig->has_lock($conf, 'backup');
 	}
+
+	vm_mon_cmd($vmid, $resume_cmd);
     });
 }
 
@@ -6300,7 +6293,7 @@ sub qemu_drive_mirror_monitor {
 			eval { PVE::QemuServer::vm_mon_cmd($vmid, "guest-fsfreeze-thaw"); };
 		    } else {
 			print "resume vm\n";
-			eval {  PVE::QemuServer::vm_resume($vmid, 1, 1); };
+			eval { vm_resume($vmid, 1) };
 		    }
 
 		    last;
-- 
2.17.1