* [PATCH-SERIES qemu-server 0/3] fix #7743: api: disk import: avoid locking twice when importing from OVA or same VM
@ 2026-06-29 12:45 Fiona Ebner
2026-06-29 12:45 ` [PATCH qemu-server 1/3] api: disk import: remove unused wrongly named variable Fiona Ebner
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Fiona Ebner @ 2026-06-29 12:45 UTC (permalink / raw)
To: pve-devel
When import_from_volid->() is called, the VM configuration of the
destination VM is already locked. In case the source volume belongs to
the same VM, there would be a second attempt to lock the config. In
particular, this also happens when importing from OVA, because the
source image is first extracted and belongs to the same VM.
Additionally, the update VM API call is changed to fork before
locking using the usual early+repeated checks pattern.
qemu-server:
Fiona Ebner (3):
api: disk import: remove unused wrongly named variable
fix #7743: api: disk import: avoid locking twice when importing from
OVA or same VM
api: update vm: fork before locking
src/PVE/API2/Qemu.pm | 780 ++++++++++++++++++++++---------------------
1 file changed, 397 insertions(+), 383 deletions(-)
Summary over all repositories:
1 files changed, 397 insertions(+), 383 deletions(-)
--
Generated by git-murpp 0.5.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH qemu-server 1/3] api: disk import: remove unused wrongly named variable
2026-06-29 12:45 [PATCH-SERIES qemu-server 0/3] fix #7743: api: disk import: avoid locking twice when importing from OVA or same VM Fiona Ebner
@ 2026-06-29 12:45 ` Fiona Ebner
2026-07-03 9:32 ` Dominik Csapak
2026-06-29 12:45 ` [PATCH qemu-server 2/3] fix #7743: api: disk import: avoid locking twice when importing from OVA or same VM Fiona Ebner
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Fiona Ebner @ 2026-06-29 12:45 UTC (permalink / raw)
To: pve-devel
The result from parse_volume_id() is the storage ID and the volume
name. Its input is the volume ID.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
src/PVE/API2/Qemu.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index 9023c344..e575d36b 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -499,7 +499,7 @@ my sub create_disks : prototype($$$$$$$$$$$) {
$needs_creation = $live_import;
- my ($source_storage, $source_volid) = PVE::Storage::parse_volume_id($source, 1);
+ my ($source_storage) = PVE::Storage::parse_volume_id($source, 1);
if ($source_storage) { # PVE-managed volume
my ($vtype, $source_format) =
--
2.47.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH qemu-server 2/3] fix #7743: api: disk import: avoid locking twice when importing from OVA or same VM
2026-06-29 12:45 [PATCH-SERIES qemu-server 0/3] fix #7743: api: disk import: avoid locking twice when importing from OVA or same VM Fiona Ebner
2026-06-29 12:45 ` [PATCH qemu-server 1/3] api: disk import: remove unused wrongly named variable Fiona Ebner
@ 2026-06-29 12:45 ` Fiona Ebner
2026-07-03 9:31 ` Dominik Csapak
2026-06-29 12:45 ` [PATCH qemu-server 3/3] api: update vm: fork before locking Fiona Ebner
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Fiona Ebner @ 2026-06-29 12:45 UTC (permalink / raw)
To: pve-devel
When import_from_volid->() is called, the VM configuration of the
destination VM is already locked. In case the source volume belongs to
the same VM, there would be a second attempt to lock the config. In
particular, this also happens when importing from OVA, because the
source image is first extracted and belongs to the same VM.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
src/PVE/API2/Qemu.pm | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index e575d36b..b2a15d96 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -390,10 +390,13 @@ my $import_from_volid = sub {
};
my $cloned;
- if ($running) {
- $cloned = PVE::QemuConfig->lock_config_full($src_vmid, 30, $clonefn);
- } elsif ($src_vmid) {
- $cloned = PVE::QemuConfig->lock_config_shared($src_vmid, 30, $clonefn);
+ # The config is already locked for the destination.
+ if ($src_vmid && $src_vmid != $dest_info->{vmid}) {
+ if ($running) {
+ $cloned = PVE::QemuConfig->lock_config_full($src_vmid, 30, $clonefn);
+ } else {
+ $cloned = PVE::QemuConfig->lock_config_shared($src_vmid, 30, $clonefn);
+ }
} else {
$cloned = $clonefn->();
}
--
2.47.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH qemu-server 3/3] api: update vm: fork before locking
2026-06-29 12:45 [PATCH-SERIES qemu-server 0/3] fix #7743: api: disk import: avoid locking twice when importing from OVA or same VM Fiona Ebner
2026-06-29 12:45 ` [PATCH qemu-server 1/3] api: disk import: remove unused wrongly named variable Fiona Ebner
2026-06-29 12:45 ` [PATCH qemu-server 2/3] fix #7743: api: disk import: avoid locking twice when importing from OVA or same VM Fiona Ebner
@ 2026-06-29 12:45 ` Fiona Ebner
2026-07-03 9:41 ` Dominik Csapak
2026-06-30 9:49 ` [PATCH-SERIES qemu-server 0/3] fix #7743: api: disk import: avoid locking twice when importing from OVA or same VM Manuel Federanko
2026-07-03 11:59 ` superseded: " Fiona Ebner
4 siblings, 1 reply; 11+ messages in thread
From: Fiona Ebner @ 2026-06-29 12:45 UTC (permalink / raw)
To: pve-devel
Use the familiar early+repeated checks pattern from other API calls to
do the forking before locking. The only intended functional changes
are with regard to locking/forking.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
NOTE: best viewed with --ignore-all-space
src/PVE/API2/Qemu.pm | 767 ++++++++++++++++++++++---------------------
1 file changed, 389 insertions(+), 378 deletions(-)
diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index b2a15d96..2e818127 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -2172,7 +2172,8 @@ my $update_vm_api = sub {
PVE::QemuServer::check_bridge_access($rpcenv, $authuser, $param);
- my $updatefn = sub {
+ my $load_and_check_config = sub {
+ my ($update_delete) = @_;
my $conf = PVE::QemuConfig->load_config($vmid);
@@ -2181,15 +2182,17 @@ my $update_vm_api = sub {
&$check_cpu_model_access($rpcenv, $authuser, $param, $conf);
- # FIXME: 'suspended' lock should probabyl be a state or "weak" lock?!
+ # FIXME: 'suspended' lock should probably be a state or "weak" lock?!
if (scalar(@delete) && grep { $_ eq 'vmstate' } @delete) {
if (defined($conf->{lock}) && $conf->{lock} eq 'suspended') {
delete $conf->{lock}; # for check lock check, not written out
- push @delete, 'lock'; # this is the real deal to write it out
+ push @delete, 'lock' if $update_delete; # this is the real deal to write it out
+ }
+ if ($update_delete) {
+ push @delete, 'runningmachine' if $conf->{runningmachine};
+ push @delete, 'runningcpu' if $conf->{runningcpu};
+ push @delete, 'running-nets-host-mtu' if $conf->{'running-nets-host-mtu'};
}
- push @delete, 'runningmachine' if $conf->{runningmachine};
- push @delete, 'runningcpu' if $conf->{runningcpu};
- push @delete, 'running-nets-host-mtu' if $conf->{'running-nets-host-mtu'};
}
PVE::QemuConfig->check_lock($conf) if !$skiplock;
@@ -2198,7 +2201,7 @@ my $update_vm_api = sub {
if (defined($conf->{$opt})) {
$param->{$opt} = $conf->{$opt};
} elsif (defined($conf->{pending}->{$opt})) {
- push @delete, $opt;
+ push @delete, $opt if $update_delete;
}
}
@@ -2214,379 +2217,387 @@ my $update_vm_api = sub {
if $balloon && $balloon > $maxmem;
}
- PVE::Cluster::log_msg('info', $authuser, "update VM $vmid: " . join(' ', @paramarr));
-
- my $worker = sub {
-
- print "update VM $vmid: " . join(' ', @paramarr) . "\n";
-
- # write updates to pending section
-
- my $modified = {}; # record what $option we modify
-
- my @bootorder;
- if (my $boot = $conf->{boot}) {
- my $bootcfg = PVE::JSONSchema::parse_property_string('pve-qm-boot', $boot);
- @bootorder = PVE::Tools::split_list($bootcfg->{order})
- if $bootcfg && $bootcfg->{order};
- }
- my $bootorder_deleted = grep { $_ eq 'bootorder' } @delete;
-
- my $check_drive_perms = sub {
- my ($opt, $val) = @_;
- my $drive = PVE::QemuServer::parse_drive($opt, $val, 1);
- if (PVE::QemuServer::drive_is_cloudinit($drive)) {
- $rpcenv->check_vm_perm(
- $authuser,
- $vmid,
- undef,
- ['VM.Config.Cloudinit', 'VM.Config.CDROM'],
- );
- } elsif (PVE::QemuServer::drive_is_cdrom($drive, 1)) { # CDROM
- $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM']);
- } else {
- $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk']);
-
- }
- };
-
- foreach my $opt (@delete) {
- $modified->{$opt} = 1;
- $conf = PVE::QemuConfig->load_config($vmid); # update/reload
-
- # value of what we want to delete, independent if pending or not
- my $val = $conf->{$opt} // $conf->{pending}->{$opt};
- if (!defined($val)) {
- warn "cannot delete '$opt' - not set in current configuration!\n";
- $modified->{$opt} = 0;
- next;
- }
- my $is_pending_val = defined($conf->{pending}->{$opt});
- delete $conf->{pending}->{$opt};
-
- # remove from bootorder if necessary
- if (!$bootorder_deleted && @bootorder && grep { $_ eq $opt } @bootorder) {
- @bootorder = grep { $_ ne $opt } @bootorder;
- $conf->{pending}->{boot} = PVE::QemuServer::print_bootorder(\@bootorder);
- $modified->{boot} = 1;
- }
-
- if ($opt =~ m/^unused/) {
- my $drive = PVE::QemuServer::parse_drive($opt, $val);
- PVE::QemuConfig->check_protection(
- $conf,
- "can't remove unused disk '$drive->{file}'",
- );
- $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk']);
- if (PVE::QemuServer::try_deallocate_drive(
- $storecfg, $vmid, $conf, $opt, $drive, $rpcenv, $authuser,
- )) {
- delete $conf->{$opt};
- PVE::QemuConfig->write_config($vmid, $conf);
- }
- } elsif ($opt eq 'vmstate') {
- PVE::QemuConfig->check_protection($conf, "can't remove vmstate '$val'");
- if (PVE::QemuServer::try_deallocate_drive(
- $storecfg,
- $vmid,
- $conf,
- $opt,
- { file => $val },
- $rpcenv,
- $authuser,
- 1,
- )) {
- delete $conf->{$opt};
- PVE::QemuConfig->write_config($vmid, $conf);
- }
- } elsif (PVE::QemuServer::is_valid_drivename($opt)) {
- PVE::QemuConfig->check_protection($conf, "can't remove drive '$opt'");
- $check_drive_perms->($opt, $val);
- PVE::QemuServer::vmconfig_register_unused_drive(
- $storecfg,
- $vmid,
- $conf,
- PVE::QemuServer::parse_drive($opt, $val),
- ) if $is_pending_val;
- PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
- PVE::QemuConfig->write_config($vmid, $conf);
- } elsif ($opt =~ m/^serial\d+$/) {
- if ($val eq 'socket') {
- $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
- } elsif ($authuser ne 'root@pam') {
- die "only root can delete '$opt' config for real devices\n";
- }
- PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
- PVE::QemuConfig->write_config($vmid, $conf);
- } elsif ($opt =~ m/^usb\d+$/) {
- check_usb_perm($rpcenv, $authuser, $vmid, undef, $opt, $val);
- PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
- PVE::QemuConfig->write_config($vmid, $conf);
- } elsif ($opt =~ m/^hostpci\d+$/) {
- check_hostpci_perm($rpcenv, $authuser, $vmid, undef, $opt, $val);
- PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
- PVE::QemuConfig->write_config($vmid, $conf);
- } elsif ($opt =~ m/^rng\d+$/) {
- check_rng_perm($rpcenv, $authuser, $vmid, undef, $opt, $val);
- PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
- PVE::QemuConfig->write_config($vmid, $conf);
- } elsif ($opt =~ m/^virtiofs\d$/) {
- check_dir_perm($rpcenv, $authuser, $vmid, undef, $opt, $val);
- PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
- PVE::QemuConfig->write_config($vmid, $conf);
- } elsif ($opt eq 'tags') {
- assert_tag_permissions($vmid, $val, '', $rpcenv, $authuser);
- delete $conf->{$opt};
- PVE::QemuConfig->write_config($vmid, $conf);
- } elsif ($opt =~ m/^net\d+$/) {
- if ($conf->{$opt}) {
- PVE::QemuServer::check_bridge_access(
- $rpcenv,
- $authuser,
- { $opt => $conf->{$opt} },
- );
- }
- PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
- PVE::QemuConfig->write_config($vmid, $conf);
- } else {
- PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
- PVE::QemuConfig->write_config($vmid, $conf);
- }
- }
-
- foreach my $opt (keys %$param) { # add/change
- $modified->{$opt} = 1;
- $conf = PVE::QemuConfig->load_config($vmid); # update/reload
- next
- if defined($conf->{pending}->{$opt})
- && ($param->{$opt} eq $conf->{pending}->{$opt}); # skip if nothing changed
-
- my $arch = PVE::QemuServer::Helpers::get_vm_arch($conf);
-
- if (PVE::QemuServer::is_valid_drivename($opt)) {
- # old drive
- if ($conf->{$opt}) {
- $check_drive_perms->($opt, $conf->{$opt});
- prohibit_tpm_version_change($conf->{$opt}, $param->{$opt})
- if $opt eq 'tpmstate0';
- }
-
- # new drive
- $check_drive_perms->($opt, $param->{$opt});
- PVE::QemuServer::vmconfig_register_unused_drive(
- $storecfg,
- $vmid,
- $conf,
- PVE::QemuServer::parse_drive($opt, $conf->{pending}->{$opt}),
- ) if defined($conf->{pending}->{$opt});
-
- assert_scsi_feature_compatibility($opt, $conf, $storecfg, $param->{$opt})
- if $opt =~ m/^scsi\d+$/;
-
- my (undef, $created_opts) = create_disks(
- $rpcenv,
- $authuser,
- $conf,
- $arch,
- $storecfg,
- $vmid,
- undef,
- { $opt => $param->{$opt} },
- undef,
- undef,
- $extraction_storage,
- );
- $conf->{pending}->{$_} = $created_opts->{$_} for keys $created_opts->%*;
-
- # default legacy boot order implies all cdroms anyway
- if (@bootorder) {
- # append new CD drives to bootorder to mark them bootable
- my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
- if (
- PVE::QemuServer::drive_is_cdrom($drive, 1)
- && !grep(/^$opt$/, @bootorder)
- ) {
- push @bootorder, $opt;
- $conf->{pending}->{boot} =
- PVE::QemuServer::print_bootorder(\@bootorder);
- $modified->{boot} = 1;
- }
- }
- } elsif ($opt =~ m/^serial\d+/) {
- if (
- (!defined($conf->{$opt}) || $conf->{$opt} eq 'socket')
- && $param->{$opt} eq 'socket'
- ) {
- $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
- } elsif ($authuser ne 'root@pam') {
- die "only root can modify '$opt' config for real devices\n";
- }
- $conf->{pending}->{$opt} = $param->{$opt};
- } elsif ($opt eq 'vga') {
- my $vga = PVE::QemuServer::parse_vga($param->{$opt});
- PVE::QemuServer::assert_clipboard_config($vga);
- $conf->{pending}->{$opt} = $param->{$opt};
- } elsif ($opt =~ m/^usb\d+/) {
- if (my $olddevice = $conf->{$opt}) {
- check_usb_perm($rpcenv, $authuser, $vmid, undef, $opt, $conf->{$opt});
- }
- check_usb_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt});
- $conf->{pending}->{$opt} = $param->{$opt};
- } elsif ($opt =~ m/^hostpci\d+$/) {
- if (my $oldvalue = $conf->{$opt}) {
- check_hostpci_perm($rpcenv, $authuser, $vmid, undef, $opt, $oldvalue);
- }
- check_hostpci_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt});
- $conf->{pending}->{$opt} = $param->{$opt};
- } elsif ($opt =~ m/^rng\d+$/) {
- if (my $oldvalue = $conf->{$opt}) {
- check_rng_perm($rpcenv, $authuser, $vmid, undef, $opt, $oldvalue);
- }
- check_rng_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt});
- $conf->{pending}->{$opt} = $param->{$opt};
- } elsif ($opt =~ m/^virtiofs\d$/) {
- if (my $oldvalue = $conf->{$opt}) {
- check_dir_perm($rpcenv, $authuser, $vmid, undef, $opt, $oldvalue);
- }
- check_dir_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt});
- $conf->{pending}->{$opt} = $param->{$opt};
- } elsif ($opt eq 'tags') {
- assert_tag_permissions(
- $vmid, $conf->{$opt}, $param->{$opt}, $rpcenv, $authuser,
- );
- $conf->{pending}->{$opt} =
- PVE::GuestHelpers::get_unique_tags($param->{$opt});
- } elsif ($opt =~ m/^net\d+$/) {
- if ($conf->{$opt}) {
- PVE::QemuServer::check_bridge_access(
- $rpcenv,
- $authuser,
- { $opt => $conf->{$opt} },
- );
- }
- $conf->{pending}->{$opt} = $param->{$opt};
- } elsif ($opt eq 'machine') {
- my $machine_conf = PVE::QemuServer::Machine::parse_machine($param->{$opt});
- PVE::QemuServer::Machine::assert_valid_machine_property($machine_conf);
- $conf->{pending}->{$opt} = $param->{$opt};
- } elsif ($opt eq 'ostype') {
- # Check if machine version pinning is needed when switching OS type, just like
- # upon creation. Skip if 'machine' is explicitly set or removed at the same time
- # to honor the users request. While it should be enough to look at $modified,
- # because 'machine' is sorted before 'ostype', be explicit just to be sure.
- if (
- !defined($param->{machine})
- && !defined($conf->{pending}->{machine})
- && !$modified->{machine} # detects deletion
- ) {
- eval {
- my $machine_string =
- PVE::QemuServer::Machine::check_and_pin_machine_string(
- $conf->{machine}, $param->{ostype}, $arch,
- );
- $conf->{pending}->{machine} = $machine_string if $machine_string;
- };
- print "automatic pinning of machine version failed - $@" if $@;
- }
- $conf->{pending}->{$opt} = $param->{$opt};
- } elsif ($opt eq 'cipassword') {
- if (!PVE::QemuServer::Helpers::windows_version($conf->{ostype})) {
- # Same logic as in cloud-init (but with the regex fixed...)
- $param->{cipassword} = PVE::Tools::encrypt_pw($param->{cipassword})
- if $param->{cipassword} !~ /^\$(?:[156]|2[ay])(\$.+){2}/;
- }
- $conf->{cipassword} = $param->{cipassword};
- } else {
- $conf->{pending}->{$opt} = $param->{$opt};
-
- if ($opt eq 'boot') {
- my $new_bootcfg =
- PVE::JSONSchema::parse_property_string('pve-qm-boot', $param->{$opt});
- if ($new_bootcfg->{order}) {
- my @devs = PVE::Tools::split_list($new_bootcfg->{order});
- for my $dev (@devs) {
- my $exists =
- $conf->{$dev} || $conf->{pending}->{$dev} || $param->{$dev};
- my $deleted = grep { $_ eq $dev } @delete;
- die "invalid bootorder: device '$dev' does not exist'\n"
- if !$exists || $deleted;
- }
-
- # remove legacy boot order settings if new one set
- $conf->{pending}->{$opt} = PVE::QemuServer::print_bootorder(\@devs);
- PVE::QemuConfig->add_to_pending_delete($conf, "bootdisk")
- if $conf->{bootdisk};
- }
- }
- }
- PVE::QemuConfig->remove_from_pending_delete($conf, $opt);
- PVE::QemuConfig->write_config($vmid, $conf);
- }
-
- # remove pending changes when nothing changed
- $conf = PVE::QemuConfig->load_config($vmid); # update/reload
- my $changes = PVE::QemuConfig->cleanup_pending($conf);
- PVE::QemuConfig->write_config($vmid, $conf) if $changes;
-
- return if !scalar(keys %{ $conf->{pending} });
-
- my $running = PVE::QemuServer::check_running($vmid);
-
- # apply pending changes
-
- $conf = PVE::QemuConfig->load_config($vmid); # update/reload
-
- my $errors = {};
- if ($running) {
- PVE::QemuServer::vmconfig_hotplug_pending(
- $vmid, $conf, $storecfg, $modified, $errors,
- );
- } else {
- # cloud_init must be skipped if we are in an incoming, remote live migration
- PVE::QemuServer::vmconfig_apply_pending(
- $vmid, $conf, $storecfg, $errors, $skip_cloud_init,
- );
- }
- raise_param_exc($errors) if scalar(keys %$errors);
-
- return;
- };
-
- if ($sync) {
- &$worker();
- return;
- } else {
- my $upid = $rpcenv->fork_worker('qmconfig', $vmid, $authuser, $worker);
-
- if ($background_delay) {
-
- # Note: It would be better to do that in the Event based HTTPServer
- # to avoid blocking call to sleep.
-
- my $end_time = time() + $background_delay;
-
- my $task = PVE::Tools::upid_decode($upid);
-
- my $running = 1;
- while (time() < $end_time) {
- $running =
- PVE::ProcFSTools::check_process_running($task->{pid}, $task->{pstart});
- last if !$running;
- sleep(1); # this gets interrupted when child process ends
- }
-
- if (!$running) {
- my $status = PVE::Tools::upid_read_status($upid);
- return if !PVE::Tools::upid_status_is_error($status);
- die "failed to update VM $vmid: $status\n";
- }
- }
-
- return $upid;
- }
+ return $conf;
};
- return PVE::QemuConfig->lock_config($vmid, $updatefn);
+ my $updatefn = sub {
+ my $conf = $load_and_check_config->(1);
+
+ PVE::Cluster::log_msg('info', $authuser, "update VM $vmid: " . join(' ', @paramarr));
+
+ print "update VM $vmid: " . join(' ', @paramarr) . "\n";
+
+ # write updates to pending section
+
+ my $modified = {}; # record what $option we modify
+
+ my @bootorder;
+ if (my $boot = $conf->{boot}) {
+ my $bootcfg = PVE::JSONSchema::parse_property_string('pve-qm-boot', $boot);
+ @bootorder = PVE::Tools::split_list($bootcfg->{order})
+ if $bootcfg && $bootcfg->{order};
+ }
+ my $bootorder_deleted = grep { $_ eq 'bootorder' } @delete;
+
+ my $check_drive_perms = sub {
+ my ($opt, $val) = @_;
+ my $drive = PVE::QemuServer::parse_drive($opt, $val, 1);
+ if (PVE::QemuServer::drive_is_cloudinit($drive)) {
+ $rpcenv->check_vm_perm(
+ $authuser,
+ $vmid,
+ undef,
+ ['VM.Config.Cloudinit', 'VM.Config.CDROM'],
+ );
+ } elsif (PVE::QemuServer::drive_is_cdrom($drive, 1)) { # CDROM
+ $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM']);
+ } else {
+ $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk']);
+
+ }
+ };
+
+ foreach my $opt (@delete) {
+ $modified->{$opt} = 1;
+ $conf = PVE::QemuConfig->load_config($vmid); # update/reload
+
+ # value of what we want to delete, independent if pending or not
+ my $val = $conf->{$opt} // $conf->{pending}->{$opt};
+ if (!defined($val)) {
+ warn "cannot delete '$opt' - not set in current configuration!\n";
+ $modified->{$opt} = 0;
+ next;
+ }
+ my $is_pending_val = defined($conf->{pending}->{$opt});
+ delete $conf->{pending}->{$opt};
+
+ # remove from bootorder if necessary
+ if (!$bootorder_deleted && @bootorder && grep { $_ eq $opt } @bootorder) {
+ @bootorder = grep { $_ ne $opt } @bootorder;
+ $conf->{pending}->{boot} = PVE::QemuServer::print_bootorder(\@bootorder);
+ $modified->{boot} = 1;
+ }
+
+ if ($opt =~ m/^unused/) {
+ my $drive = PVE::QemuServer::parse_drive($opt, $val);
+ PVE::QemuConfig->check_protection(
+ $conf,
+ "can't remove unused disk '$drive->{file}'",
+ );
+ $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk']);
+ if (PVE::QemuServer::try_deallocate_drive(
+ $storecfg, $vmid, $conf, $opt, $drive, $rpcenv, $authuser,
+ )) {
+ delete $conf->{$opt};
+ PVE::QemuConfig->write_config($vmid, $conf);
+ }
+ } elsif ($opt eq 'vmstate') {
+ PVE::QemuConfig->check_protection($conf, "can't remove vmstate '$val'");
+ if (PVE::QemuServer::try_deallocate_drive(
+ $storecfg,
+ $vmid,
+ $conf,
+ $opt,
+ { file => $val },
+ $rpcenv,
+ $authuser,
+ 1,
+ )) {
+ delete $conf->{$opt};
+ PVE::QemuConfig->write_config($vmid, $conf);
+ }
+ } elsif (PVE::QemuServer::is_valid_drivename($opt)) {
+ PVE::QemuConfig->check_protection($conf, "can't remove drive '$opt'");
+ $check_drive_perms->($opt, $val);
+ PVE::QemuServer::vmconfig_register_unused_drive(
+ $storecfg,
+ $vmid,
+ $conf,
+ PVE::QemuServer::parse_drive($opt, $val),
+ ) if $is_pending_val;
+ PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
+ PVE::QemuConfig->write_config($vmid, $conf);
+ } elsif ($opt =~ m/^serial\d+$/) {
+ if ($val eq 'socket') {
+ $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
+ } elsif ($authuser ne 'root@pam') {
+ die "only root can delete '$opt' config for real devices\n";
+ }
+ PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
+ PVE::QemuConfig->write_config($vmid, $conf);
+ } elsif ($opt =~ m/^usb\d+$/) {
+ check_usb_perm($rpcenv, $authuser, $vmid, undef, $opt, $val);
+ PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
+ PVE::QemuConfig->write_config($vmid, $conf);
+ } elsif ($opt =~ m/^hostpci\d+$/) {
+ check_hostpci_perm($rpcenv, $authuser, $vmid, undef, $opt, $val);
+ PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
+ PVE::QemuConfig->write_config($vmid, $conf);
+ } elsif ($opt =~ m/^rng\d+$/) {
+ check_rng_perm($rpcenv, $authuser, $vmid, undef, $opt, $val);
+ PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
+ PVE::QemuConfig->write_config($vmid, $conf);
+ } elsif ($opt =~ m/^virtiofs\d$/) {
+ check_dir_perm($rpcenv, $authuser, $vmid, undef, $opt, $val);
+ PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
+ PVE::QemuConfig->write_config($vmid, $conf);
+ } elsif ($opt eq 'tags') {
+ assert_tag_permissions($vmid, $val, '', $rpcenv, $authuser);
+ delete $conf->{$opt};
+ PVE::QemuConfig->write_config($vmid, $conf);
+ } elsif ($opt =~ m/^net\d+$/) {
+ if ($conf->{$opt}) {
+ PVE::QemuServer::check_bridge_access(
+ $rpcenv,
+ $authuser,
+ { $opt => $conf->{$opt} },
+ );
+ }
+ PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
+ PVE::QemuConfig->write_config($vmid, $conf);
+ } else {
+ PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
+ PVE::QemuConfig->write_config($vmid, $conf);
+ }
+ }
+
+ foreach my $opt (keys %$param) { # add/change
+ $modified->{$opt} = 1;
+ $conf = PVE::QemuConfig->load_config($vmid); # update/reload
+ next
+ if defined($conf->{pending}->{$opt})
+ && ($param->{$opt} eq $conf->{pending}->{$opt}); # skip if nothing changed
+
+ my $arch = PVE::QemuServer::Helpers::get_vm_arch($conf);
+
+ if (PVE::QemuServer::is_valid_drivename($opt)) {
+ # old drive
+ if ($conf->{$opt}) {
+ $check_drive_perms->($opt, $conf->{$opt});
+ prohibit_tpm_version_change($conf->{$opt}, $param->{$opt})
+ if $opt eq 'tpmstate0';
+ }
+
+ # new drive
+ $check_drive_perms->($opt, $param->{$opt});
+ PVE::QemuServer::vmconfig_register_unused_drive(
+ $storecfg,
+ $vmid,
+ $conf,
+ PVE::QemuServer::parse_drive($opt, $conf->{pending}->{$opt}),
+ ) if defined($conf->{pending}->{$opt});
+
+ assert_scsi_feature_compatibility($opt, $conf, $storecfg, $param->{$opt})
+ if $opt =~ m/^scsi\d+$/;
+
+ my (undef, $created_opts) = create_disks(
+ $rpcenv,
+ $authuser,
+ $conf,
+ $arch,
+ $storecfg,
+ $vmid,
+ undef,
+ { $opt => $param->{$opt} },
+ undef,
+ undef,
+ $extraction_storage,
+ );
+ $conf->{pending}->{$_} = $created_opts->{$_} for keys $created_opts->%*;
+
+ # default legacy boot order implies all cdroms anyway
+ if (@bootorder) {
+ # append new CD drives to bootorder to mark them bootable
+ my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
+ if (
+ PVE::QemuServer::drive_is_cdrom($drive, 1)
+ && !grep(/^$opt$/, @bootorder)
+ ) {
+ push @bootorder, $opt;
+ $conf->{pending}->{boot} =
+ PVE::QemuServer::print_bootorder(\@bootorder);
+ $modified->{boot} = 1;
+ }
+ }
+ } elsif ($opt =~ m/^serial\d+/) {
+ if (
+ (!defined($conf->{$opt}) || $conf->{$opt} eq 'socket')
+ && $param->{$opt} eq 'socket'
+ ) {
+ $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
+ } elsif ($authuser ne 'root@pam') {
+ die "only root can modify '$opt' config for real devices\n";
+ }
+ $conf->{pending}->{$opt} = $param->{$opt};
+ } elsif ($opt eq 'vga') {
+ my $vga = PVE::QemuServer::parse_vga($param->{$opt});
+ PVE::QemuServer::assert_clipboard_config($vga);
+ $conf->{pending}->{$opt} = $param->{$opt};
+ } elsif ($opt =~ m/^usb\d+/) {
+ if (my $olddevice = $conf->{$opt}) {
+ check_usb_perm($rpcenv, $authuser, $vmid, undef, $opt, $conf->{$opt});
+ }
+ check_usb_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt});
+ $conf->{pending}->{$opt} = $param->{$opt};
+ } elsif ($opt =~ m/^hostpci\d+$/) {
+ if (my $oldvalue = $conf->{$opt}) {
+ check_hostpci_perm($rpcenv, $authuser, $vmid, undef, $opt, $oldvalue);
+ }
+ check_hostpci_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt});
+ $conf->{pending}->{$opt} = $param->{$opt};
+ } elsif ($opt =~ m/^rng\d+$/) {
+ if (my $oldvalue = $conf->{$opt}) {
+ check_rng_perm($rpcenv, $authuser, $vmid, undef, $opt, $oldvalue);
+ }
+ check_rng_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt});
+ $conf->{pending}->{$opt} = $param->{$opt};
+ } elsif ($opt =~ m/^virtiofs\d$/) {
+ if (my $oldvalue = $conf->{$opt}) {
+ check_dir_perm($rpcenv, $authuser, $vmid, undef, $opt, $oldvalue);
+ }
+ check_dir_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt});
+ $conf->{pending}->{$opt} = $param->{$opt};
+ } elsif ($opt eq 'tags') {
+ assert_tag_permissions(
+ $vmid, $conf->{$opt}, $param->{$opt}, $rpcenv, $authuser,
+ );
+ $conf->{pending}->{$opt} =
+ PVE::GuestHelpers::get_unique_tags($param->{$opt});
+ } elsif ($opt =~ m/^net\d+$/) {
+ if ($conf->{$opt}) {
+ PVE::QemuServer::check_bridge_access(
+ $rpcenv,
+ $authuser,
+ { $opt => $conf->{$opt} },
+ );
+ }
+ $conf->{pending}->{$opt} = $param->{$opt};
+ } elsif ($opt eq 'machine') {
+ my $machine_conf = PVE::QemuServer::Machine::parse_machine($param->{$opt});
+ PVE::QemuServer::Machine::assert_valid_machine_property($machine_conf);
+ $conf->{pending}->{$opt} = $param->{$opt};
+ } elsif ($opt eq 'ostype') {
+ # Check if machine version pinning is needed when switching OS type, just like
+ # upon creation. Skip if 'machine' is explicitly set or removed at the same time
+ # to honor the users request. While it should be enough to look at $modified,
+ # because 'machine' is sorted before 'ostype', be explicit just to be sure.
+ if (
+ !defined($param->{machine})
+ && !defined($conf->{pending}->{machine})
+ && !$modified->{machine} # detects deletion
+ ) {
+ eval {
+ my $machine_string =
+ PVE::QemuServer::Machine::check_and_pin_machine_string(
+ $conf->{machine}, $param->{ostype}, $arch,
+ );
+ $conf->{pending}->{machine} = $machine_string if $machine_string;
+ };
+ print "automatic pinning of machine version failed - $@" if $@;
+ }
+ $conf->{pending}->{$opt} = $param->{$opt};
+ } elsif ($opt eq 'cipassword') {
+ if (!PVE::QemuServer::Helpers::windows_version($conf->{ostype})) {
+ # Same logic as in cloud-init (but with the regex fixed...)
+ $param->{cipassword} = PVE::Tools::encrypt_pw($param->{cipassword})
+ if $param->{cipassword} !~ /^\$(?:[156]|2[ay])(\$.+){2}/;
+ }
+ $conf->{cipassword} = $param->{cipassword};
+ } else {
+ $conf->{pending}->{$opt} = $param->{$opt};
+
+ if ($opt eq 'boot') {
+ my $new_bootcfg =
+ PVE::JSONSchema::parse_property_string('pve-qm-boot', $param->{$opt});
+ if ($new_bootcfg->{order}) {
+ my @devs = PVE::Tools::split_list($new_bootcfg->{order});
+ for my $dev (@devs) {
+ my $exists =
+ $conf->{$dev} || $conf->{pending}->{$dev} || $param->{$dev};
+ my $deleted = grep { $_ eq $dev } @delete;
+ die "invalid bootorder: device '$dev' does not exist'\n"
+ if !$exists || $deleted;
+ }
+
+ # remove legacy boot order settings if new one set
+ $conf->{pending}->{$opt} = PVE::QemuServer::print_bootorder(\@devs);
+ PVE::QemuConfig->add_to_pending_delete($conf, "bootdisk")
+ if $conf->{bootdisk};
+ }
+ }
+ }
+ PVE::QemuConfig->remove_from_pending_delete($conf, $opt);
+ PVE::QemuConfig->write_config($vmid, $conf);
+ }
+
+ # remove pending changes when nothing changed
+ $conf = PVE::QemuConfig->load_config($vmid); # update/reload
+ my $changes = PVE::QemuConfig->cleanup_pending($conf);
+ PVE::QemuConfig->write_config($vmid, $conf) if $changes;
+
+ return if !scalar(keys %{ $conf->{pending} });
+
+ my $running = PVE::QemuServer::check_running($vmid);
+
+ # apply pending changes
+
+ $conf = PVE::QemuConfig->load_config($vmid); # update/reload
+
+ my $errors = {};
+ if ($running) {
+ PVE::QemuServer::vmconfig_hotplug_pending(
+ $vmid, $conf, $storecfg, $modified, $errors,
+ );
+ } else {
+ # cloud_init must be skipped if we are in an incoming, remote live migration
+ PVE::QemuServer::vmconfig_apply_pending(
+ $vmid, $conf, $storecfg, $errors, $skip_cloud_init,
+ );
+ }
+ raise_param_exc($errors) if scalar(keys %$errors);
+
+ return;
+ };
+
+ $load_and_check_config->(0); # early checks before forking/locking
+
+ if ($sync) {
+ PVE::QemuConfig->lock_config($vmid, $updatefn);
+ return;
+ } else {
+ my $upid = $rpcenv->fork_worker(
+ 'qmconfig',
+ $vmid,
+ $authuser,
+ sub { PVE::QemuConfig->lock_config($vmid, $updatefn); },
+ );
+
+ if ($background_delay) {
+
+ # Note: It would be better to do that in the Event based HTTPServer
+ # to avoid blocking call to sleep.
+
+ my $end_time = time() + $background_delay;
+
+ my $task = PVE::Tools::upid_decode($upid);
+
+ my $running = 1;
+ while (time() < $end_time) {
+ $running =
+ PVE::ProcFSTools::check_process_running($task->{pid}, $task->{pstart});
+ last if !$running;
+ sleep(1); # this gets interrupted when child process ends
+ }
+
+ if (!$running) {
+ my $status = PVE::Tools::upid_read_status($upid);
+ return if !PVE::Tools::upid_status_is_error($status);
+ die "failed to update VM $vmid: $status\n";
+ }
+ }
+
+ return $upid;
+ }
};
my $vm_config_perm_list = [
--
2.47.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH-SERIES qemu-server 0/3] fix #7743: api: disk import: avoid locking twice when importing from OVA or same VM
2026-06-29 12:45 [PATCH-SERIES qemu-server 0/3] fix #7743: api: disk import: avoid locking twice when importing from OVA or same VM Fiona Ebner
` (2 preceding siblings ...)
2026-06-29 12:45 ` [PATCH qemu-server 3/3] api: update vm: fork before locking Fiona Ebner
@ 2026-06-30 9:49 ` Manuel Federanko
2026-07-03 11:59 ` superseded: " Fiona Ebner
4 siblings, 0 replies; 11+ messages in thread
From: Manuel Federanko @ 2026-06-30 9:49 UTC (permalink / raw)
To: pve-devel
Hi,
I built the package, installed it and check that the issue no longer
exists with my reproducer [0].
Everything seems to be working fine.
Thanks!
Tested-by: Manuel Federanko <m.federanko@proxmox.com>
[0] https://bugzilla.proxmox.com/show_bug.cgi?id=7743
On 2026-06-29 2:46 PM, Fiona Ebner wrote:
> When import_from_volid->() is called, the VM configuration of the
> destination VM is already locked. In case the source volume belongs to
> the same VM, there would be a second attempt to lock the config. In
> particular, this also happens when importing from OVA, because the
> source image is first extracted and belongs to the same VM.
>
> Additionally, the update VM API call is changed to fork before
> locking using the usual early+repeated checks pattern.
>
> qemu-server:
>
> Fiona Ebner (3):
> api: disk import: remove unused wrongly named variable
> fix #7743: api: disk import: avoid locking twice when importing from
> OVA or same VM
> api: update vm: fork before locking
>
> src/PVE/API2/Qemu.pm | 780 ++++++++++++++++++++++---------------------
> 1 file changed, 397 insertions(+), 383 deletions(-)
>
>
> Summary over all repositories:
> 1 files changed, 397 insertions(+), 383 deletions(-)
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH qemu-server 2/3] fix #7743: api: disk import: avoid locking twice when importing from OVA or same VM
2026-06-29 12:45 ` [PATCH qemu-server 2/3] fix #7743: api: disk import: avoid locking twice when importing from OVA or same VM Fiona Ebner
@ 2026-07-03 9:31 ` Dominik Csapak
0 siblings, 0 replies; 11+ messages in thread
From: Dominik Csapak @ 2026-07-03 9:31 UTC (permalink / raw)
To: Fiona Ebner, pve-devel
On 6/29/26 2:46 PM, Fiona Ebner wrote:
> When import_from_volid->() is called, the VM configuration of the
> destination VM is already locked. In case the source volume belongs to
> the same VM, there would be a second attempt to lock the config. In
> particular, this also happens when importing from OVA, because the
> source image is first extracted and belongs to the same VM.
the explanation makes sense, and it fixes the issue (works now for POST
& PUT) but i'm somehow missing the connection here?
AFAIU the reason is that in the async case we lock the config
then fork and then lock again (which does not work)
but in the sync case we lock and later lock again in the
same process so the cached lock in PVE::Tools succeeds?
IMHO the code and rationale is fine here, but i would find it good
to get the actual reason why it failed + why this change
impacts the POST vs PUT case.
Otherwise from the commit message consider this
Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> src/PVE/API2/Qemu.pm | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
> index e575d36b..b2a15d96 100644
> --- a/src/PVE/API2/Qemu.pm
> +++ b/src/PVE/API2/Qemu.pm
> @@ -390,10 +390,13 @@ my $import_from_volid = sub {
> };
>
> my $cloned;
> - if ($running) {
> - $cloned = PVE::QemuConfig->lock_config_full($src_vmid, 30, $clonefn);
> - } elsif ($src_vmid) {
> - $cloned = PVE::QemuConfig->lock_config_shared($src_vmid, 30, $clonefn);
> + # The config is already locked for the destination.
> + if ($src_vmid && $src_vmid != $dest_info->{vmid}) {
> + if ($running) {
> + $cloned = PVE::QemuConfig->lock_config_full($src_vmid, 30, $clonefn);
> + } else {
> + $cloned = PVE::QemuConfig->lock_config_shared($src_vmid, 30, $clonefn);
> + }
> } else {
> $cloned = $clonefn->();
> }
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH qemu-server 1/3] api: disk import: remove unused wrongly named variable
2026-06-29 12:45 ` [PATCH qemu-server 1/3] api: disk import: remove unused wrongly named variable Fiona Ebner
@ 2026-07-03 9:32 ` Dominik Csapak
0 siblings, 0 replies; 11+ messages in thread
From: Dominik Csapak @ 2026-07-03 9:32 UTC (permalink / raw)
To: Fiona Ebner, pve-devel
Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
On 6/29/26 2:46 PM, Fiona Ebner wrote:
> The result from parse_volume_id() is the storage ID and the volume
> name. Its input is the volume ID.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> src/PVE/API2/Qemu.pm | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
> index 9023c344..e575d36b 100644
> --- a/src/PVE/API2/Qemu.pm
> +++ b/src/PVE/API2/Qemu.pm
> @@ -499,7 +499,7 @@ my sub create_disks : prototype($$$$$$$$$$$) {
>
> $needs_creation = $live_import;
>
> - my ($source_storage, $source_volid) = PVE::Storage::parse_volume_id($source, 1);
> + my ($source_storage) = PVE::Storage::parse_volume_id($source, 1);
>
> if ($source_storage) { # PVE-managed volume
> my ($vtype, $source_format) =
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH qemu-server 3/3] api: update vm: fork before locking
2026-06-29 12:45 ` [PATCH qemu-server 3/3] api: update vm: fork before locking Fiona Ebner
@ 2026-07-03 9:41 ` Dominik Csapak
2026-07-03 11:44 ` Fiona Ebner
0 siblings, 1 reply; 11+ messages in thread
From: Dominik Csapak @ 2026-07-03 9:41 UTC (permalink / raw)
To: Fiona Ebner, pve-devel
one remark/question:
would it make for a nicer interface for the load_and_check_config
if we'd pass the delete list into it instead of using
catching the outer closure variable?
that way we wouldn't have to guard the 'push @delete' statements
and could simply pass an empty list into it when we don't want
to add them.
the only difference it would make is that inside, we wouldn't
delete the $conf->{lock} there but since we discard that
before using it when we don't want to update delete this
should be fine?
but this is not super important and the code is fine as is too
so with or without this changed consider this
Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
On 6/29/26 2:47 PM, Fiona Ebner wrote:
> Use the familiar early+repeated checks pattern from other API calls to
> do the forking before locking. The only intended functional changes
> are with regard to locking/forking.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>
> NOTE: best viewed with --ignore-all-space
>
> src/PVE/API2/Qemu.pm | 767 ++++++++++++++++++++++---------------------
> 1 file changed, 389 insertions(+), 378 deletions(-)
>
> diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
> index b2a15d96..2e818127 100644
> --- a/src/PVE/API2/Qemu.pm
> +++ b/src/PVE/API2/Qemu.pm
> @@ -2172,7 +2172,8 @@ my $update_vm_api = sub {
>
> PVE::QemuServer::check_bridge_access($rpcenv, $authuser, $param);
>
> - my $updatefn = sub {
> + my $load_and_check_config = sub {
> + my ($update_delete) = @_;
>
> my $conf = PVE::QemuConfig->load_config($vmid);
>
> @@ -2181,15 +2182,17 @@ my $update_vm_api = sub {
>
> &$check_cpu_model_access($rpcenv, $authuser, $param, $conf);
>
> - # FIXME: 'suspended' lock should probabyl be a state or "weak" lock?!
> + # FIXME: 'suspended' lock should probably be a state or "weak" lock?!
> if (scalar(@delete) && grep { $_ eq 'vmstate' } @delete) {
> if (defined($conf->{lock}) && $conf->{lock} eq 'suspended') {
> delete $conf->{lock}; # for check lock check, not written out
> - push @delete, 'lock'; # this is the real deal to write it out
> + push @delete, 'lock' if $update_delete; # this is the real deal to write it out
> + }
> + if ($update_delete) {
> + push @delete, 'runningmachine' if $conf->{runningmachine};
> + push @delete, 'runningcpu' if $conf->{runningcpu};
> + push @delete, 'running-nets-host-mtu' if $conf->{'running-nets-host-mtu'};
> }
> - push @delete, 'runningmachine' if $conf->{runningmachine};
> - push @delete, 'runningcpu' if $conf->{runningcpu};
> - push @delete, 'running-nets-host-mtu' if $conf->{'running-nets-host-mtu'};
> }
>
> PVE::QemuConfig->check_lock($conf) if !$skiplock;
> @@ -2198,7 +2201,7 @@ my $update_vm_api = sub {
> if (defined($conf->{$opt})) {
> $param->{$opt} = $conf->{$opt};
> } elsif (defined($conf->{pending}->{$opt})) {
> - push @delete, $opt;
> + push @delete, $opt if $update_delete;
> }
> }
>
> @@ -2214,379 +2217,387 @@ my $update_vm_api = sub {
> if $balloon && $balloon > $maxmem;
> }
>
> - PVE::Cluster::log_msg('info', $authuser, "update VM $vmid: " . join(' ', @paramarr));
> -
> - my $worker = sub {
> -
> - print "update VM $vmid: " . join(' ', @paramarr) . "\n";
> -
> - # write updates to pending section
> -
> - my $modified = {}; # record what $option we modify
> -
> - my @bootorder;
> - if (my $boot = $conf->{boot}) {
> - my $bootcfg = PVE::JSONSchema::parse_property_string('pve-qm-boot', $boot);
> - @bootorder = PVE::Tools::split_list($bootcfg->{order})
> - if $bootcfg && $bootcfg->{order};
> - }
> - my $bootorder_deleted = grep { $_ eq 'bootorder' } @delete;
> -
> - my $check_drive_perms = sub {
> - my ($opt, $val) = @_;
> - my $drive = PVE::QemuServer::parse_drive($opt, $val, 1);
> - if (PVE::QemuServer::drive_is_cloudinit($drive)) {
> - $rpcenv->check_vm_perm(
> - $authuser,
> - $vmid,
> - undef,
> - ['VM.Config.Cloudinit', 'VM.Config.CDROM'],
> - );
> - } elsif (PVE::QemuServer::drive_is_cdrom($drive, 1)) { # CDROM
> - $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM']);
> - } else {
> - $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk']);
> -
> - }
> - };
> -
> - foreach my $opt (@delete) {
> - $modified->{$opt} = 1;
> - $conf = PVE::QemuConfig->load_config($vmid); # update/reload
> -
> - # value of what we want to delete, independent if pending or not
> - my $val = $conf->{$opt} // $conf->{pending}->{$opt};
> - if (!defined($val)) {
> - warn "cannot delete '$opt' - not set in current configuration!\n";
> - $modified->{$opt} = 0;
> - next;
> - }
> - my $is_pending_val = defined($conf->{pending}->{$opt});
> - delete $conf->{pending}->{$opt};
> -
> - # remove from bootorder if necessary
> - if (!$bootorder_deleted && @bootorder && grep { $_ eq $opt } @bootorder) {
> - @bootorder = grep { $_ ne $opt } @bootorder;
> - $conf->{pending}->{boot} = PVE::QemuServer::print_bootorder(\@bootorder);
> - $modified->{boot} = 1;
> - }
> -
> - if ($opt =~ m/^unused/) {
> - my $drive = PVE::QemuServer::parse_drive($opt, $val);
> - PVE::QemuConfig->check_protection(
> - $conf,
> - "can't remove unused disk '$drive->{file}'",
> - );
> - $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk']);
> - if (PVE::QemuServer::try_deallocate_drive(
> - $storecfg, $vmid, $conf, $opt, $drive, $rpcenv, $authuser,
> - )) {
> - delete $conf->{$opt};
> - PVE::QemuConfig->write_config($vmid, $conf);
> - }
> - } elsif ($opt eq 'vmstate') {
> - PVE::QemuConfig->check_protection($conf, "can't remove vmstate '$val'");
> - if (PVE::QemuServer::try_deallocate_drive(
> - $storecfg,
> - $vmid,
> - $conf,
> - $opt,
> - { file => $val },
> - $rpcenv,
> - $authuser,
> - 1,
> - )) {
> - delete $conf->{$opt};
> - PVE::QemuConfig->write_config($vmid, $conf);
> - }
> - } elsif (PVE::QemuServer::is_valid_drivename($opt)) {
> - PVE::QemuConfig->check_protection($conf, "can't remove drive '$opt'");
> - $check_drive_perms->($opt, $val);
> - PVE::QemuServer::vmconfig_register_unused_drive(
> - $storecfg,
> - $vmid,
> - $conf,
> - PVE::QemuServer::parse_drive($opt, $val),
> - ) if $is_pending_val;
> - PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
> - PVE::QemuConfig->write_config($vmid, $conf);
> - } elsif ($opt =~ m/^serial\d+$/) {
> - if ($val eq 'socket') {
> - $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> - } elsif ($authuser ne 'root@pam') {
> - die "only root can delete '$opt' config for real devices\n";
> - }
> - PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
> - PVE::QemuConfig->write_config($vmid, $conf);
> - } elsif ($opt =~ m/^usb\d+$/) {
> - check_usb_perm($rpcenv, $authuser, $vmid, undef, $opt, $val);
> - PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
> - PVE::QemuConfig->write_config($vmid, $conf);
> - } elsif ($opt =~ m/^hostpci\d+$/) {
> - check_hostpci_perm($rpcenv, $authuser, $vmid, undef, $opt, $val);
> - PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
> - PVE::QemuConfig->write_config($vmid, $conf);
> - } elsif ($opt =~ m/^rng\d+$/) {
> - check_rng_perm($rpcenv, $authuser, $vmid, undef, $opt, $val);
> - PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
> - PVE::QemuConfig->write_config($vmid, $conf);
> - } elsif ($opt =~ m/^virtiofs\d$/) {
> - check_dir_perm($rpcenv, $authuser, $vmid, undef, $opt, $val);
> - PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
> - PVE::QemuConfig->write_config($vmid, $conf);
> - } elsif ($opt eq 'tags') {
> - assert_tag_permissions($vmid, $val, '', $rpcenv, $authuser);
> - delete $conf->{$opt};
> - PVE::QemuConfig->write_config($vmid, $conf);
> - } elsif ($opt =~ m/^net\d+$/) {
> - if ($conf->{$opt}) {
> - PVE::QemuServer::check_bridge_access(
> - $rpcenv,
> - $authuser,
> - { $opt => $conf->{$opt} },
> - );
> - }
> - PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
> - PVE::QemuConfig->write_config($vmid, $conf);
> - } else {
> - PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
> - PVE::QemuConfig->write_config($vmid, $conf);
> - }
> - }
> -
> - foreach my $opt (keys %$param) { # add/change
> - $modified->{$opt} = 1;
> - $conf = PVE::QemuConfig->load_config($vmid); # update/reload
> - next
> - if defined($conf->{pending}->{$opt})
> - && ($param->{$opt} eq $conf->{pending}->{$opt}); # skip if nothing changed
> -
> - my $arch = PVE::QemuServer::Helpers::get_vm_arch($conf);
> -
> - if (PVE::QemuServer::is_valid_drivename($opt)) {
> - # old drive
> - if ($conf->{$opt}) {
> - $check_drive_perms->($opt, $conf->{$opt});
> - prohibit_tpm_version_change($conf->{$opt}, $param->{$opt})
> - if $opt eq 'tpmstate0';
> - }
> -
> - # new drive
> - $check_drive_perms->($opt, $param->{$opt});
> - PVE::QemuServer::vmconfig_register_unused_drive(
> - $storecfg,
> - $vmid,
> - $conf,
> - PVE::QemuServer::parse_drive($opt, $conf->{pending}->{$opt}),
> - ) if defined($conf->{pending}->{$opt});
> -
> - assert_scsi_feature_compatibility($opt, $conf, $storecfg, $param->{$opt})
> - if $opt =~ m/^scsi\d+$/;
> -
> - my (undef, $created_opts) = create_disks(
> - $rpcenv,
> - $authuser,
> - $conf,
> - $arch,
> - $storecfg,
> - $vmid,
> - undef,
> - { $opt => $param->{$opt} },
> - undef,
> - undef,
> - $extraction_storage,
> - );
> - $conf->{pending}->{$_} = $created_opts->{$_} for keys $created_opts->%*;
> -
> - # default legacy boot order implies all cdroms anyway
> - if (@bootorder) {
> - # append new CD drives to bootorder to mark them bootable
> - my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
> - if (
> - PVE::QemuServer::drive_is_cdrom($drive, 1)
> - && !grep(/^$opt$/, @bootorder)
> - ) {
> - push @bootorder, $opt;
> - $conf->{pending}->{boot} =
> - PVE::QemuServer::print_bootorder(\@bootorder);
> - $modified->{boot} = 1;
> - }
> - }
> - } elsif ($opt =~ m/^serial\d+/) {
> - if (
> - (!defined($conf->{$opt}) || $conf->{$opt} eq 'socket')
> - && $param->{$opt} eq 'socket'
> - ) {
> - $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> - } elsif ($authuser ne 'root@pam') {
> - die "only root can modify '$opt' config for real devices\n";
> - }
> - $conf->{pending}->{$opt} = $param->{$opt};
> - } elsif ($opt eq 'vga') {
> - my $vga = PVE::QemuServer::parse_vga($param->{$opt});
> - PVE::QemuServer::assert_clipboard_config($vga);
> - $conf->{pending}->{$opt} = $param->{$opt};
> - } elsif ($opt =~ m/^usb\d+/) {
> - if (my $olddevice = $conf->{$opt}) {
> - check_usb_perm($rpcenv, $authuser, $vmid, undef, $opt, $conf->{$opt});
> - }
> - check_usb_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt});
> - $conf->{pending}->{$opt} = $param->{$opt};
> - } elsif ($opt =~ m/^hostpci\d+$/) {
> - if (my $oldvalue = $conf->{$opt}) {
> - check_hostpci_perm($rpcenv, $authuser, $vmid, undef, $opt, $oldvalue);
> - }
> - check_hostpci_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt});
> - $conf->{pending}->{$opt} = $param->{$opt};
> - } elsif ($opt =~ m/^rng\d+$/) {
> - if (my $oldvalue = $conf->{$opt}) {
> - check_rng_perm($rpcenv, $authuser, $vmid, undef, $opt, $oldvalue);
> - }
> - check_rng_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt});
> - $conf->{pending}->{$opt} = $param->{$opt};
> - } elsif ($opt =~ m/^virtiofs\d$/) {
> - if (my $oldvalue = $conf->{$opt}) {
> - check_dir_perm($rpcenv, $authuser, $vmid, undef, $opt, $oldvalue);
> - }
> - check_dir_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt});
> - $conf->{pending}->{$opt} = $param->{$opt};
> - } elsif ($opt eq 'tags') {
> - assert_tag_permissions(
> - $vmid, $conf->{$opt}, $param->{$opt}, $rpcenv, $authuser,
> - );
> - $conf->{pending}->{$opt} =
> - PVE::GuestHelpers::get_unique_tags($param->{$opt});
> - } elsif ($opt =~ m/^net\d+$/) {
> - if ($conf->{$opt}) {
> - PVE::QemuServer::check_bridge_access(
> - $rpcenv,
> - $authuser,
> - { $opt => $conf->{$opt} },
> - );
> - }
> - $conf->{pending}->{$opt} = $param->{$opt};
> - } elsif ($opt eq 'machine') {
> - my $machine_conf = PVE::QemuServer::Machine::parse_machine($param->{$opt});
> - PVE::QemuServer::Machine::assert_valid_machine_property($machine_conf);
> - $conf->{pending}->{$opt} = $param->{$opt};
> - } elsif ($opt eq 'ostype') {
> - # Check if machine version pinning is needed when switching OS type, just like
> - # upon creation. Skip if 'machine' is explicitly set or removed at the same time
> - # to honor the users request. While it should be enough to look at $modified,
> - # because 'machine' is sorted before 'ostype', be explicit just to be sure.
> - if (
> - !defined($param->{machine})
> - && !defined($conf->{pending}->{machine})
> - && !$modified->{machine} # detects deletion
> - ) {
> - eval {
> - my $machine_string =
> - PVE::QemuServer::Machine::check_and_pin_machine_string(
> - $conf->{machine}, $param->{ostype}, $arch,
> - );
> - $conf->{pending}->{machine} = $machine_string if $machine_string;
> - };
> - print "automatic pinning of machine version failed - $@" if $@;
> - }
> - $conf->{pending}->{$opt} = $param->{$opt};
> - } elsif ($opt eq 'cipassword') {
> - if (!PVE::QemuServer::Helpers::windows_version($conf->{ostype})) {
> - # Same logic as in cloud-init (but with the regex fixed...)
> - $param->{cipassword} = PVE::Tools::encrypt_pw($param->{cipassword})
> - if $param->{cipassword} !~ /^\$(?:[156]|2[ay])(\$.+){2}/;
> - }
> - $conf->{cipassword} = $param->{cipassword};
> - } else {
> - $conf->{pending}->{$opt} = $param->{$opt};
> -
> - if ($opt eq 'boot') {
> - my $new_bootcfg =
> - PVE::JSONSchema::parse_property_string('pve-qm-boot', $param->{$opt});
> - if ($new_bootcfg->{order}) {
> - my @devs = PVE::Tools::split_list($new_bootcfg->{order});
> - for my $dev (@devs) {
> - my $exists =
> - $conf->{$dev} || $conf->{pending}->{$dev} || $param->{$dev};
> - my $deleted = grep { $_ eq $dev } @delete;
> - die "invalid bootorder: device '$dev' does not exist'\n"
> - if !$exists || $deleted;
> - }
> -
> - # remove legacy boot order settings if new one set
> - $conf->{pending}->{$opt} = PVE::QemuServer::print_bootorder(\@devs);
> - PVE::QemuConfig->add_to_pending_delete($conf, "bootdisk")
> - if $conf->{bootdisk};
> - }
> - }
> - }
> - PVE::QemuConfig->remove_from_pending_delete($conf, $opt);
> - PVE::QemuConfig->write_config($vmid, $conf);
> - }
> -
> - # remove pending changes when nothing changed
> - $conf = PVE::QemuConfig->load_config($vmid); # update/reload
> - my $changes = PVE::QemuConfig->cleanup_pending($conf);
> - PVE::QemuConfig->write_config($vmid, $conf) if $changes;
> -
> - return if !scalar(keys %{ $conf->{pending} });
> -
> - my $running = PVE::QemuServer::check_running($vmid);
> -
> - # apply pending changes
> -
> - $conf = PVE::QemuConfig->load_config($vmid); # update/reload
> -
> - my $errors = {};
> - if ($running) {
> - PVE::QemuServer::vmconfig_hotplug_pending(
> - $vmid, $conf, $storecfg, $modified, $errors,
> - );
> - } else {
> - # cloud_init must be skipped if we are in an incoming, remote live migration
> - PVE::QemuServer::vmconfig_apply_pending(
> - $vmid, $conf, $storecfg, $errors, $skip_cloud_init,
> - );
> - }
> - raise_param_exc($errors) if scalar(keys %$errors);
> -
> - return;
> - };
> -
> - if ($sync) {
> - &$worker();
> - return;
> - } else {
> - my $upid = $rpcenv->fork_worker('qmconfig', $vmid, $authuser, $worker);
> -
> - if ($background_delay) {
> -
> - # Note: It would be better to do that in the Event based HTTPServer
> - # to avoid blocking call to sleep.
> -
> - my $end_time = time() + $background_delay;
> -
> - my $task = PVE::Tools::upid_decode($upid);
> -
> - my $running = 1;
> - while (time() < $end_time) {
> - $running =
> - PVE::ProcFSTools::check_process_running($task->{pid}, $task->{pstart});
> - last if !$running;
> - sleep(1); # this gets interrupted when child process ends
> - }
> -
> - if (!$running) {
> - my $status = PVE::Tools::upid_read_status($upid);
> - return if !PVE::Tools::upid_status_is_error($status);
> - die "failed to update VM $vmid: $status\n";
> - }
> - }
> -
> - return $upid;
> - }
> + return $conf;
> };
>
> - return PVE::QemuConfig->lock_config($vmid, $updatefn);
> + my $updatefn = sub {
> + my $conf = $load_and_check_config->(1);
> +
> + PVE::Cluster::log_msg('info', $authuser, "update VM $vmid: " . join(' ', @paramarr));
> +
> + print "update VM $vmid: " . join(' ', @paramarr) . "\n";
> +
> + # write updates to pending section
> +
> + my $modified = {}; # record what $option we modify
> +
> + my @bootorder;
> + if (my $boot = $conf->{boot}) {
> + my $bootcfg = PVE::JSONSchema::parse_property_string('pve-qm-boot', $boot);
> + @bootorder = PVE::Tools::split_list($bootcfg->{order})
> + if $bootcfg && $bootcfg->{order};
> + }
> + my $bootorder_deleted = grep { $_ eq 'bootorder' } @delete;
> +
> + my $check_drive_perms = sub {
> + my ($opt, $val) = @_;
> + my $drive = PVE::QemuServer::parse_drive($opt, $val, 1);
> + if (PVE::QemuServer::drive_is_cloudinit($drive)) {
> + $rpcenv->check_vm_perm(
> + $authuser,
> + $vmid,
> + undef,
> + ['VM.Config.Cloudinit', 'VM.Config.CDROM'],
> + );
> + } elsif (PVE::QemuServer::drive_is_cdrom($drive, 1)) { # CDROM
> + $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM']);
> + } else {
> + $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk']);
> +
> + }
> + };
> +
> + foreach my $opt (@delete) {
> + $modified->{$opt} = 1;
> + $conf = PVE::QemuConfig->load_config($vmid); # update/reload
> +
> + # value of what we want to delete, independent if pending or not
> + my $val = $conf->{$opt} // $conf->{pending}->{$opt};
> + if (!defined($val)) {
> + warn "cannot delete '$opt' - not set in current configuration!\n";
> + $modified->{$opt} = 0;
> + next;
> + }
> + my $is_pending_val = defined($conf->{pending}->{$opt});
> + delete $conf->{pending}->{$opt};
> +
> + # remove from bootorder if necessary
> + if (!$bootorder_deleted && @bootorder && grep { $_ eq $opt } @bootorder) {
> + @bootorder = grep { $_ ne $opt } @bootorder;
> + $conf->{pending}->{boot} = PVE::QemuServer::print_bootorder(\@bootorder);
> + $modified->{boot} = 1;
> + }
> +
> + if ($opt =~ m/^unused/) {
> + my $drive = PVE::QemuServer::parse_drive($opt, $val);
> + PVE::QemuConfig->check_protection(
> + $conf,
> + "can't remove unused disk '$drive->{file}'",
> + );
> + $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk']);
> + if (PVE::QemuServer::try_deallocate_drive(
> + $storecfg, $vmid, $conf, $opt, $drive, $rpcenv, $authuser,
> + )) {
> + delete $conf->{$opt};
> + PVE::QemuConfig->write_config($vmid, $conf);
> + }
> + } elsif ($opt eq 'vmstate') {
> + PVE::QemuConfig->check_protection($conf, "can't remove vmstate '$val'");
> + if (PVE::QemuServer::try_deallocate_drive(
> + $storecfg,
> + $vmid,
> + $conf,
> + $opt,
> + { file => $val },
> + $rpcenv,
> + $authuser,
> + 1,
> + )) {
> + delete $conf->{$opt};
> + PVE::QemuConfig->write_config($vmid, $conf);
> + }
> + } elsif (PVE::QemuServer::is_valid_drivename($opt)) {
> + PVE::QemuConfig->check_protection($conf, "can't remove drive '$opt'");
> + $check_drive_perms->($opt, $val);
> + PVE::QemuServer::vmconfig_register_unused_drive(
> + $storecfg,
> + $vmid,
> + $conf,
> + PVE::QemuServer::parse_drive($opt, $val),
> + ) if $is_pending_val;
> + PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
> + PVE::QemuConfig->write_config($vmid, $conf);
> + } elsif ($opt =~ m/^serial\d+$/) {
> + if ($val eq 'socket') {
> + $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> + } elsif ($authuser ne 'root@pam') {
> + die "only root can delete '$opt' config for real devices\n";
> + }
> + PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
> + PVE::QemuConfig->write_config($vmid, $conf);
> + } elsif ($opt =~ m/^usb\d+$/) {
> + check_usb_perm($rpcenv, $authuser, $vmid, undef, $opt, $val);
> + PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
> + PVE::QemuConfig->write_config($vmid, $conf);
> + } elsif ($opt =~ m/^hostpci\d+$/) {
> + check_hostpci_perm($rpcenv, $authuser, $vmid, undef, $opt, $val);
> + PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
> + PVE::QemuConfig->write_config($vmid, $conf);
> + } elsif ($opt =~ m/^rng\d+$/) {
> + check_rng_perm($rpcenv, $authuser, $vmid, undef, $opt, $val);
> + PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
> + PVE::QemuConfig->write_config($vmid, $conf);
> + } elsif ($opt =~ m/^virtiofs\d$/) {
> + check_dir_perm($rpcenv, $authuser, $vmid, undef, $opt, $val);
> + PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
> + PVE::QemuConfig->write_config($vmid, $conf);
> + } elsif ($opt eq 'tags') {
> + assert_tag_permissions($vmid, $val, '', $rpcenv, $authuser);
> + delete $conf->{$opt};
> + PVE::QemuConfig->write_config($vmid, $conf);
> + } elsif ($opt =~ m/^net\d+$/) {
> + if ($conf->{$opt}) {
> + PVE::QemuServer::check_bridge_access(
> + $rpcenv,
> + $authuser,
> + { $opt => $conf->{$opt} },
> + );
> + }
> + PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
> + PVE::QemuConfig->write_config($vmid, $conf);
> + } else {
> + PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
> + PVE::QemuConfig->write_config($vmid, $conf);
> + }
> + }
> +
> + foreach my $opt (keys %$param) { # add/change
> + $modified->{$opt} = 1;
> + $conf = PVE::QemuConfig->load_config($vmid); # update/reload
> + next
> + if defined($conf->{pending}->{$opt})
> + && ($param->{$opt} eq $conf->{pending}->{$opt}); # skip if nothing changed
> +
> + my $arch = PVE::QemuServer::Helpers::get_vm_arch($conf);
> +
> + if (PVE::QemuServer::is_valid_drivename($opt)) {
> + # old drive
> + if ($conf->{$opt}) {
> + $check_drive_perms->($opt, $conf->{$opt});
> + prohibit_tpm_version_change($conf->{$opt}, $param->{$opt})
> + if $opt eq 'tpmstate0';
> + }
> +
> + # new drive
> + $check_drive_perms->($opt, $param->{$opt});
> + PVE::QemuServer::vmconfig_register_unused_drive(
> + $storecfg,
> + $vmid,
> + $conf,
> + PVE::QemuServer::parse_drive($opt, $conf->{pending}->{$opt}),
> + ) if defined($conf->{pending}->{$opt});
> +
> + assert_scsi_feature_compatibility($opt, $conf, $storecfg, $param->{$opt})
> + if $opt =~ m/^scsi\d+$/;
> +
> + my (undef, $created_opts) = create_disks(
> + $rpcenv,
> + $authuser,
> + $conf,
> + $arch,
> + $storecfg,
> + $vmid,
> + undef,
> + { $opt => $param->{$opt} },
> + undef,
> + undef,
> + $extraction_storage,
> + );
> + $conf->{pending}->{$_} = $created_opts->{$_} for keys $created_opts->%*;
> +
> + # default legacy boot order implies all cdroms anyway
> + if (@bootorder) {
> + # append new CD drives to bootorder to mark them bootable
> + my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
> + if (
> + PVE::QemuServer::drive_is_cdrom($drive, 1)
> + && !grep(/^$opt$/, @bootorder)
> + ) {
> + push @bootorder, $opt;
> + $conf->{pending}->{boot} =
> + PVE::QemuServer::print_bootorder(\@bootorder);
> + $modified->{boot} = 1;
> + }
> + }
> + } elsif ($opt =~ m/^serial\d+/) {
> + if (
> + (!defined($conf->{$opt}) || $conf->{$opt} eq 'socket')
> + && $param->{$opt} eq 'socket'
> + ) {
> + $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> + } elsif ($authuser ne 'root@pam') {
> + die "only root can modify '$opt' config for real devices\n";
> + }
> + $conf->{pending}->{$opt} = $param->{$opt};
> + } elsif ($opt eq 'vga') {
> + my $vga = PVE::QemuServer::parse_vga($param->{$opt});
> + PVE::QemuServer::assert_clipboard_config($vga);
> + $conf->{pending}->{$opt} = $param->{$opt};
> + } elsif ($opt =~ m/^usb\d+/) {
> + if (my $olddevice = $conf->{$opt}) {
> + check_usb_perm($rpcenv, $authuser, $vmid, undef, $opt, $conf->{$opt});
> + }
> + check_usb_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt});
> + $conf->{pending}->{$opt} = $param->{$opt};
> + } elsif ($opt =~ m/^hostpci\d+$/) {
> + if (my $oldvalue = $conf->{$opt}) {
> + check_hostpci_perm($rpcenv, $authuser, $vmid, undef, $opt, $oldvalue);
> + }
> + check_hostpci_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt});
> + $conf->{pending}->{$opt} = $param->{$opt};
> + } elsif ($opt =~ m/^rng\d+$/) {
> + if (my $oldvalue = $conf->{$opt}) {
> + check_rng_perm($rpcenv, $authuser, $vmid, undef, $opt, $oldvalue);
> + }
> + check_rng_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt});
> + $conf->{pending}->{$opt} = $param->{$opt};
> + } elsif ($opt =~ m/^virtiofs\d$/) {
> + if (my $oldvalue = $conf->{$opt}) {
> + check_dir_perm($rpcenv, $authuser, $vmid, undef, $opt, $oldvalue);
> + }
> + check_dir_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt});
> + $conf->{pending}->{$opt} = $param->{$opt};
> + } elsif ($opt eq 'tags') {
> + assert_tag_permissions(
> + $vmid, $conf->{$opt}, $param->{$opt}, $rpcenv, $authuser,
> + );
> + $conf->{pending}->{$opt} =
> + PVE::GuestHelpers::get_unique_tags($param->{$opt});
> + } elsif ($opt =~ m/^net\d+$/) {
> + if ($conf->{$opt}) {
> + PVE::QemuServer::check_bridge_access(
> + $rpcenv,
> + $authuser,
> + { $opt => $conf->{$opt} },
> + );
> + }
> + $conf->{pending}->{$opt} = $param->{$opt};
> + } elsif ($opt eq 'machine') {
> + my $machine_conf = PVE::QemuServer::Machine::parse_machine($param->{$opt});
> + PVE::QemuServer::Machine::assert_valid_machine_property($machine_conf);
> + $conf->{pending}->{$opt} = $param->{$opt};
> + } elsif ($opt eq 'ostype') {
> + # Check if machine version pinning is needed when switching OS type, just like
> + # upon creation. Skip if 'machine' is explicitly set or removed at the same time
> + # to honor the users request. While it should be enough to look at $modified,
> + # because 'machine' is sorted before 'ostype', be explicit just to be sure.
> + if (
> + !defined($param->{machine})
> + && !defined($conf->{pending}->{machine})
> + && !$modified->{machine} # detects deletion
> + ) {
> + eval {
> + my $machine_string =
> + PVE::QemuServer::Machine::check_and_pin_machine_string(
> + $conf->{machine}, $param->{ostype}, $arch,
> + );
> + $conf->{pending}->{machine} = $machine_string if $machine_string;
> + };
> + print "automatic pinning of machine version failed - $@" if $@;
> + }
> + $conf->{pending}->{$opt} = $param->{$opt};
> + } elsif ($opt eq 'cipassword') {
> + if (!PVE::QemuServer::Helpers::windows_version($conf->{ostype})) {
> + # Same logic as in cloud-init (but with the regex fixed...)
> + $param->{cipassword} = PVE::Tools::encrypt_pw($param->{cipassword})
> + if $param->{cipassword} !~ /^\$(?:[156]|2[ay])(\$.+){2}/;
> + }
> + $conf->{cipassword} = $param->{cipassword};
> + } else {
> + $conf->{pending}->{$opt} = $param->{$opt};
> +
> + if ($opt eq 'boot') {
> + my $new_bootcfg =
> + PVE::JSONSchema::parse_property_string('pve-qm-boot', $param->{$opt});
> + if ($new_bootcfg->{order}) {
> + my @devs = PVE::Tools::split_list($new_bootcfg->{order});
> + for my $dev (@devs) {
> + my $exists =
> + $conf->{$dev} || $conf->{pending}->{$dev} || $param->{$dev};
> + my $deleted = grep { $_ eq $dev } @delete;
> + die "invalid bootorder: device '$dev' does not exist'\n"
> + if !$exists || $deleted;
> + }
> +
> + # remove legacy boot order settings if new one set
> + $conf->{pending}->{$opt} = PVE::QemuServer::print_bootorder(\@devs);
> + PVE::QemuConfig->add_to_pending_delete($conf, "bootdisk")
> + if $conf->{bootdisk};
> + }
> + }
> + }
> + PVE::QemuConfig->remove_from_pending_delete($conf, $opt);
> + PVE::QemuConfig->write_config($vmid, $conf);
> + }
> +
> + # remove pending changes when nothing changed
> + $conf = PVE::QemuConfig->load_config($vmid); # update/reload
> + my $changes = PVE::QemuConfig->cleanup_pending($conf);
> + PVE::QemuConfig->write_config($vmid, $conf) if $changes;
> +
> + return if !scalar(keys %{ $conf->{pending} });
> +
> + my $running = PVE::QemuServer::check_running($vmid);
> +
> + # apply pending changes
> +
> + $conf = PVE::QemuConfig->load_config($vmid); # update/reload
> +
> + my $errors = {};
> + if ($running) {
> + PVE::QemuServer::vmconfig_hotplug_pending(
> + $vmid, $conf, $storecfg, $modified, $errors,
> + );
> + } else {
> + # cloud_init must be skipped if we are in an incoming, remote live migration
> + PVE::QemuServer::vmconfig_apply_pending(
> + $vmid, $conf, $storecfg, $errors, $skip_cloud_init,
> + );
> + }
> + raise_param_exc($errors) if scalar(keys %$errors);
> +
> + return;
> + };
> +
> + $load_and_check_config->(0); # early checks before forking/locking
> +
> + if ($sync) {
> + PVE::QemuConfig->lock_config($vmid, $updatefn);
> + return;
> + } else {
> + my $upid = $rpcenv->fork_worker(
> + 'qmconfig',
> + $vmid,
> + $authuser,
> + sub { PVE::QemuConfig->lock_config($vmid, $updatefn); },
> + );
> +
> + if ($background_delay) {
> +
> + # Note: It would be better to do that in the Event based HTTPServer
> + # to avoid blocking call to sleep.
> +
> + my $end_time = time() + $background_delay;
> +
> + my $task = PVE::Tools::upid_decode($upid);
> +
> + my $running = 1;
> + while (time() < $end_time) {
> + $running =
> + PVE::ProcFSTools::check_process_running($task->{pid}, $task->{pstart});
> + last if !$running;
> + sleep(1); # this gets interrupted when child process ends
> + }
> +
> + if (!$running) {
> + my $status = PVE::Tools::upid_read_status($upid);
> + return if !PVE::Tools::upid_status_is_error($status);
> + die "failed to update VM $vmid: $status\n";
> + }
> + }
> +
> + return $upid;
> + }
> };
>
> my $vm_config_perm_list = [
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH qemu-server 3/3] api: update vm: fork before locking
2026-07-03 9:41 ` Dominik Csapak
@ 2026-07-03 11:44 ` Fiona Ebner
2026-07-03 12:00 ` Dominik Csapak
0 siblings, 1 reply; 11+ messages in thread
From: Fiona Ebner @ 2026-07-03 11:44 UTC (permalink / raw)
To: Dominik Csapak, pve-devel
Am 03.07.26 um 11:41 AM schrieb Dominik Csapak:
> one remark/question:
>
> would it make for a nicer interface for the load_and_check_config
> if we'd pass the delete list into it instead of using
> catching the outer closure variable?
>
> that way we wouldn't have to guard the 'push @delete' statements
> and could simply pass an empty list into it when we don't want
> to add them.
>
> the only difference it would make is that inside, we wouldn't
> delete the $conf->{lock} there but since we discard that
> before using it when we don't want to update delete this
> should be fine?
As discussed during lunch too, in case the vmstate is being deleted,
it's important that the load_and_check_config() gets the actual $delete
list. Because if we pass an empty list, then we don't delete the
suspended lock, and then the check_lock() right below would fail.
Of course, this could be handled by adapting the function with
additional arguments, but the guard seems rather straightforward to me,
so I would be inclined to just keep it like that.
^ permalink raw reply [flat|nested] 11+ messages in thread
* superseded: [PATCH-SERIES qemu-server 0/3] fix #7743: api: disk import: avoid locking twice when importing from OVA or same VM
2026-06-29 12:45 [PATCH-SERIES qemu-server 0/3] fix #7743: api: disk import: avoid locking twice when importing from OVA or same VM Fiona Ebner
` (3 preceding siblings ...)
2026-06-30 9:49 ` [PATCH-SERIES qemu-server 0/3] fix #7743: api: disk import: avoid locking twice when importing from OVA or same VM Manuel Federanko
@ 2026-07-03 11:59 ` Fiona Ebner
4 siblings, 0 replies; 11+ messages in thread
From: Fiona Ebner @ 2026-07-03 11:59 UTC (permalink / raw)
To: pve-devel
superseded-by:
https://lore.proxmox.com/pve-devel/20260703115432.108667-1-f.ebner@proxmox.com/T/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH qemu-server 3/3] api: update vm: fork before locking
2026-07-03 11:44 ` Fiona Ebner
@ 2026-07-03 12:00 ` Dominik Csapak
0 siblings, 0 replies; 11+ messages in thread
From: Dominik Csapak @ 2026-07-03 12:00 UTC (permalink / raw)
To: Fiona Ebner, pve-devel
On 7/3/26 1:44 PM, Fiona Ebner wrote:
> Am 03.07.26 um 11:41 AM schrieb Dominik Csapak:
>> one remark/question:
>>
>> would it make for a nicer interface for the load_and_check_config
>> if we'd pass the delete list into it instead of using
>> catching the outer closure variable?
>>
>> that way we wouldn't have to guard the 'push @delete' statements
>> and could simply pass an empty list into it when we don't want
>> to add them.
>>
>> the only difference it would make is that inside, we wouldn't
>> delete the $conf->{lock} there but since we discard that
>> before using it when we don't want to update delete this
>> should be fine?
>
> As discussed during lunch too, in case the vmstate is being deleted,
> it's important that the load_and_check_config() gets the actual $delete
> list. Because if we pass an empty list, then we don't delete the
> suspended lock, and then the check_lock() right below would fail.
>
> Of course, this could be handled by adapting the function with
> additional arguments, but the guard seems rather straightforward to me,
> so I would be inclined to just keep it like that.
yes, sure makes sense (i overlooked the check_lock call)
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2026-07-03 12:00 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-29 12:45 [PATCH-SERIES qemu-server 0/3] fix #7743: api: disk import: avoid locking twice when importing from OVA or same VM Fiona Ebner
2026-06-29 12:45 ` [PATCH qemu-server 1/3] api: disk import: remove unused wrongly named variable Fiona Ebner
2026-07-03 9:32 ` Dominik Csapak
2026-06-29 12:45 ` [PATCH qemu-server 2/3] fix #7743: api: disk import: avoid locking twice when importing from OVA or same VM Fiona Ebner
2026-07-03 9:31 ` Dominik Csapak
2026-06-29 12:45 ` [PATCH qemu-server 3/3] api: update vm: fork before locking Fiona Ebner
2026-07-03 9:41 ` Dominik Csapak
2026-07-03 11:44 ` Fiona Ebner
2026-07-03 12:00 ` Dominik Csapak
2026-06-30 9:49 ` [PATCH-SERIES qemu-server 0/3] fix #7743: api: disk import: avoid locking twice when importing from OVA or same VM Manuel Federanko
2026-07-03 11:59 ` superseded: " Fiona Ebner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox