[pve-devel] [PATCH qemu-server 14/31] move find_vmstate_storage() helper to QemuConfig module

From: Fiona Ebner <f.ebner@proxmox.com>
To: pve-devel@lists.proxmox.com
Subject: [pve-devel] [PATCH qemu-server 14/31] move find_vmstate_storage() helper to QemuConfig module
Date: Wed, 25 Jun 2025 17:56:37 +0200	[thread overview]
Message-ID: <20250625155751.268047-15-f.ebner@proxmox.com> (raw)
In-Reply-To: <20250625155751.268047-1-f.ebner@proxmox.com>

While not the main motivation, this has the nice side effect of
removing a call from QemuConfig to the QemuServer main module.

This is in preparation to introduce a RunState module which does not
call back into the main QemuServer module. In particular, qm_suspend()
will be moved to RunState which needs to call find_vmstate_storage().

Intuitively, the StateFile module seems like the most natural place
for find_vmstate_storage(), but moving find_vmstate_storage() requires
moving foreach_storage_used_by_vm() too and that function calls into
QemuConfig. Now, QemuConfig also calls find_vmstate_storage(), meaning
a cyclic dependency would result.

Note that foreach_storage_used_by_vm(), is related to foreach_volume()
and also uses foreach_volume(), so QemuConfig is the natural place for
that function.

So the arguments for moving find_vmstate_storage() to QemuConfig are:
1. most natural way to avoid cylcic dependencies.
2. related function foreach_storage_used_by_vm() belongs there too.
3. qm_suspend() and other functions relating to the run state already
   call other QemuConfig methods.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/API2/Qemu.pm  |  2 +-
 src/PVE/QemuConfig.pm | 52 ++++++++++++++++++++++++++++++++++++++++++-
 src/PVE/QemuServer.pm | 52 +------------------------------------------
 3 files changed, 53 insertions(+), 53 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index 747edb62..7f55998e 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -3922,7 +3922,7 @@ __PACKAGE__->register_method({
             if (!$statestorage) {
                 # get statestorage from config if none is given
                 my $storecfg = PVE::Storage::config();
-                $statestorage = PVE::QemuServer::find_vmstate_storage($conf, $storecfg);
+                $statestorage = PVE::QemuConfig::find_vmstate_storage($conf, $storecfg);
             }
 
             $rpcenv->check($authuser, "/storage/$statestorage", ['Datastore.AllocateSpace']);
diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm
index 500b4c0b..957c875a 100644
--- a/src/PVE/QemuConfig.pm
+++ b/src/PVE/QemuConfig.pm
@@ -221,7 +221,7 @@ sub __snapshot_save_vmstate {
     my $target = $statestorage;
 
     if (!$target) {
-        $target = PVE::QemuServer::find_vmstate_storage($conf, $storecfg);
+        $target = find_vmstate_storage($conf, $storecfg);
     }
 
     my $mem_size = get_current_memory($conf->{memory});
@@ -712,4 +712,54 @@ sub cleanup_fleecing_images {
     record_fleecing_images($vmid, $failed);
 }
 
+sub foreach_storage_used_by_vm {
+    my ($conf, $func) = @_;
+
+    my $sidhash = {};
+
+    PVE::QemuConfig->foreach_volume(
+        $conf,
+        sub {
+            my ($ds, $drive) = @_;
+            return if PVE::QemuServer::Drive::drive_is_cdrom($drive);
+
+            my $volid = $drive->{file};
+
+            my ($sid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
+            $sidhash->{$sid} = $sid if $sid;
+        },
+    );
+
+    foreach my $sid (sort keys %$sidhash) {
+        &$func($sid);
+    }
+}
+
+# NOTE: if this logic changes, please update docs & possibly gui logic
+sub find_vmstate_storage {
+    my ($conf, $storecfg) = @_;
+
+    # first, return storage from conf if set
+    return $conf->{vmstatestorage} if $conf->{vmstatestorage};
+
+    my ($target, $shared, $local);
+
+    foreach_storage_used_by_vm(
+        $conf,
+        sub {
+            my ($sid) = @_;
+            my $scfg = PVE::Storage::storage_config($storecfg, $sid);
+            my $dst = $scfg->{shared} ? \$shared : \$local;
+            $$dst = $sid if !$$dst || $scfg->{path}; # prefer file based storage
+        },
+    );
+
+    # second, use shared storage where VM has at least one disk
+    # third, use local storage where VM has at least one disk
+    # fall back to local storage
+    $target = $shared // $local // 'local';
+
+    return $target;
+}
+
 1;
diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 02dcc02c..d24dc7eb 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -6322,7 +6322,7 @@ sub vm_suspend {
                 my $date = strftime("%Y-%m-%d", localtime(time()));
                 $storecfg = PVE::Storage::config();
                 if (!$statestorage) {
-                    $statestorage = find_vmstate_storage($conf, $storecfg);
+                    $statestorage = PVE::QemuConfig::find_vmstate_storage($conf, $storecfg);
                     # check permissions for the storage
                     my $rpcenv = PVE::RPCEnvironment::get();
                     if ($rpcenv->{type} ne 'cli') {
@@ -7877,29 +7877,6 @@ sub restore_tar_archive {
     warn $@ if $@;
 }
 
-sub foreach_storage_used_by_vm {
-    my ($conf, $func) = @_;
-
-    my $sidhash = {};
-
-    PVE::QemuConfig->foreach_volume(
-        $conf,
-        sub {
-            my ($ds, $drive) = @_;
-            return if drive_is_cdrom($drive);
-
-            my $volid = $drive->{file};
-
-            my ($sid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
-            $sidhash->{$sid} = $sid if $sid;
-        },
-    );
-
-    foreach my $sid (sort keys %$sidhash) {
-        &$func($sid);
-    }
-}
-
 my $qemu_snap_storage = {
     rbd => 1,
 };
@@ -8558,33 +8535,6 @@ sub resolve_dst_disk_format {
     return $format;
 }
 
-# NOTE: if this logic changes, please update docs & possibly gui logic
-sub find_vmstate_storage {
-    my ($conf, $storecfg) = @_;
-
-    # first, return storage from conf if set
-    return $conf->{vmstatestorage} if $conf->{vmstatestorage};
-
-    my ($target, $shared, $local);
-
-    foreach_storage_used_by_vm(
-        $conf,
-        sub {
-            my ($sid) = @_;
-            my $scfg = PVE::Storage::storage_config($storecfg, $sid);
-            my $dst = $scfg->{shared} ? \$shared : \$local;
-            $$dst = $sid if !$$dst || $scfg->{path}; # prefer file based storage
-        },
-    );
-
-    # second, use shared storage where VM has at least one disk
-    # third, use local storage where VM has at least one disk
-    # fall back to local storage
-    $target = $shared // $local // 'local';
-
-    return $target;
-}
-
 sub generate_uuid {
     my ($uuid, $uuid_str);
     UUID::generate($uuid);
-- 
2.47.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

