From b137c30c3a5e4f5394e961a2048724fa18f86b2c Mon Sep 17 00:00:00 2001
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
Date: Wed, 16 Nov 2022 12:03:49 +0100
Subject: [PATCH] cloudinit: avoid unsafe write of VM config

there's no guarantee that we're locked here and it also produces
unnecessary extra IO in most cases.

While at it also avoid that a special:cloudinit section is added on
start to *every* VM, which caused another bug to trigger (see prev.
commit) and is just odd for users that ain't using cloudinit

Note in two call sites that we may need to write the config indeed
out there on the caller side.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 PVE/QemuServer.pm           | 20 ++++++++++++--------
 PVE/QemuServer/Cloudinit.pm | 19 ++++++++-----------
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index f82c1e68..8bebd0fb 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5018,7 +5018,7 @@ sub vmconfig_hotplug_pending {
 		# some changes can be done without hotplug
 		my $drive = parse_drive($opt, $value);
 		if (drive_is_cloudinit($drive)) {
-		    PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid);
+		    $conf->{cloudinit} = PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid);
 		}
 		vmconfig_update_disk($storecfg, $conf, $hotplug_features->{disk},
 				     $vmid, $opt, $value, $arch, $machine_type);
@@ -5060,7 +5060,7 @@ sub vmconfig_hotplug_pending {
 
     PVE::QemuConfig->write_config($vmid, $conf);
 
-    if($hotplug_features->{cloudinit}) {
+    if ($hotplug_features->{cloudinit}) {
 	my $pending = PVE::QemuServer::Cloudinit::get_pending_config($conf, $vmid);
 	my $regenerate = undef;
 	for my $item (@$pending) {
@@ -5167,9 +5167,11 @@ sub vmconfig_apply_pending {
 	}
     }
 
+    $conf->{cloudinit} = PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid)
+	if $generate_cloudnit;
+
     # write all changes at once to avoid unnecessary i/o
     PVE::QemuConfig->write_config($vmid, $conf);
-    PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid) if $generate_cloudnit;
 }
 
 sub vmconfig_update_net {
@@ -5372,7 +5374,8 @@ sub vmconfig_update_cloudinit_drive {
 
     return if !$cloudinit_drive;
 
-    PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid);
+    $conf->{cloudinit} = PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid);
+    # FIXME: write out changed config here? needs to be sure that config is locked though!
     my $running = PVE::QemuServer::check_running($vmid);
 
     if ($running) {
@@ -5554,12 +5557,13 @@ sub vm_start_nolock {
     if (!$statefile && scalar(keys %{$conf->{pending}})) {
 	vmconfig_apply_pending($vmid, $conf, $storecfg);
 	$conf = PVE::QemuConfig->load_config($vmid); # update/reload
+    } elsif (!$migratedfrom) {
+	# don't regenerate the ISO if the VM is started as part of a live migration
+	# this way we can reuse the old ISO with the correct config
+	$conf->{cloudinit} = PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid);
+	# FIXME: write out changed config here? if any changes
     }
 
-    # don't regenerate the ISO if the VM is started as part of a live migration
-    # this way we can reuse the old ISO with the correct config
-    PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid) if !$migratedfrom;
-
     # override offline migrated volumes, conf is out of date still
     if (my $offline_volumes = $migrate_opts->{offline_volumes}) {
 	for my $key (sort keys $offline_volumes->%*) {
diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
index b616c7b9..d4a34c40 100644
--- a/PVE/QemuServer/Cloudinit.pm
+++ b/PVE/QemuServer/Cloudinit.pm
@@ -566,7 +566,6 @@ sub generate_cloudinitconfig {
 
     PVE::QemuConfig->foreach_volume($conf, sub {
         my ($ds, $drive) = @_;
-
 	my ($storeid, $volname) = PVE::Storage::parse_volume_id($drive->{file}, 1);
 
 	return if !$volname || $volname !~ m/vm-$vmid-cloudinit/;
@@ -577,36 +576,34 @@ sub generate_cloudinitconfig {
 	$generator->($conf, $vmid, $drive, $volname, $storeid);
     });
 
-    my $cloudinitconf = delete $conf->{cloudinit};
-    $cloudinitconf = {};
+    my $cloudinit_conf = {};
 
     my @cloudinit_opts = keys %{PVE::QemuServer::cloudinit_config_properties()};
     push @cloudinit_opts, 'name';
 
     for my $opt (@cloudinit_opts) {
-
 	if ($opt =~ m/^ipconfig(\d+)/) {
 	    my $netid = "net$1";
 	    next if !defined($conf->{$netid});
-	    $conf->{cloudinit}->{$netid} = $conf->{$netid};
+	    $cloudinit_conf->{$netid} = $conf->{$netid};
 	}
 
-	$conf->{cloudinit}->{$opt} = $conf->{$opt} if $conf->{$opt};
+	$cloudinit_conf->{$opt} = $conf->{$opt} if $conf->{$opt};
     }
 
-    $conf->{cloudinit}->{name} = "VM$vmid" if !$conf->{cloudinit}->{name};
-
+    my $has_cloudinit_drive = 0;
     for my $opt (keys %{$conf}) {
 	if (PVE::QemuServer::is_valid_drivename($opt)) {
 	    my $drive = PVE::QemuServer::parse_drive($opt, $conf->{$opt});
 	    if (PVE::QemuServer::drive_is_cloudinit($drive)) {
-		$conf->{cloudinit}->{$opt} = $conf->{$opt};
+		$has_cloudinit_drive = 1;
+		$cloudinit_conf->{$opt} = $conf->{$opt};
 	    }
 	}
     }
+    $cloudinit_conf->{name} //= "VM$vmid" if $has_cloudinit_drive;
 
-    PVE::QemuConfig->write_config($vmid, $conf);
-
+    return $cloudinit_conf;
 }
 
 sub dump_cloudinit_config {