[PATCH-SERIES qemu-server 0/8] fix #7383: agent: fsfreeze: skip freeze if already frozen

[PATCH-SERIES qemu-server 0/8] fix #7383: agent: fsfreeze: skip freeze if already frozen

[Fiona Ebner]

The series is structured as follows:

The first patch is an independent fix for a long-standing issue that
the result of the guest-fsfreeze-thaw command was not checked.

The next two patches fix checking whether the agent is enabled for
backup and clone/import, which in practice avoids a warning about the
agent not running if explicitly disabled.

The next four patches lead up to harmonizing the checks before freeze
which previously were duplicated to three different places and improve
encapsulation of the agent module.

The last patch closes bug #7383 and checks if the filesystem is
already frozen before attempting freeze to avoid a confusing error
message.

qemu-server:

Fiona Ebner (8):
  agent: fs thaw: check command result
  api: clone/import: fix check if agent is enabled
  backup: freeze: fix check if agent is enabled
  agent: parse: change signature to take property string rather than
    full VM config
  agent: get qga key: change signature to take property string rather
    than full VM config
  clone disk/block jobs: change signatures to take guest agent property
    string
  agent: fsfreeze: harmonize checks for guest fs freeze
  fix #7383: agent: fsfreeze: skip freeze if already frozen

 src/PVE/API2/Qemu.pm           | 15 ++----
 src/PVE/QemuConfig.pm          | 14 ++----
 src/PVE/QemuMigrate.pm         |  2 +-
 src/PVE/QemuServer.pm          | 14 ++++--
 src/PVE/QemuServer/Agent.pm    | 89 +++++++++++++++++++++++++++++++---
 src/PVE/QemuServer/BlockJob.pm | 15 ++----
 src/PVE/VZDump/QemuServer.pm   | 20 +++-----
 7 files changed, 114 insertions(+), 55 deletions(-)


Summary over all repositories:
  7 files changed, 114 insertions(+), 55 deletions(-)

-- 
Generated by git-murpp 0.5.0

[PATCH qemu-server 1/8] agent: fs thaw: check command result

[Fiona Ebner]

All callers already use eval{} and warn, so this does not change the
execution flow for any callers, but thaw errors will not be quietly
ignored anymore.

Fixes: 90fde50a ("agent: add a guest_fsthaw helper")
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuServer/Agent.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/PVE/QemuServer/Agent.pm b/src/PVE/QemuServer/Agent.pm
index fb3e1835..1e7b1895 100644
--- a/src/PVE/QemuServer/Agent.pm
+++ b/src/PVE/QemuServer/Agent.pm
@@ -283,7 +283,10 @@ See C<$guest_fsfreeze> for more details.
 sub guest_fsthaw {
     my ($vmid) = @_;
 
-    PVE::QemuServer::Monitor::mon_cmd($vmid, "guest-fsfreeze-thaw");
+    my $res = PVE::QemuServer::Monitor::mon_cmd($vmid, "guest-fsfreeze-thaw");
+    check_agent_error($res, "unable to thaw guest filesystem");
+
+    return;
 }
 
 1;
-- 
2.47.3

[PATCH qemu-server 2/8] api: clone/import: fix check if agent is enabled

[Fiona Ebner]

Just checking whether the configuration is present does not mean that
the guest agent is enabled. Explicitly check the 'enabled' property.
In case of snapshot, this is already done. This avoids a log message
about the agent not running, since there is no device added to the
QEMU commandline for communicating with the agent if disabled.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/API2/Qemu.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index b409610c..1752f46a 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -374,7 +374,7 @@ my $import_from_volid = sub {
 
         my ($src_storeid) = PVE::Storage::parse_volume_id($src_volid);
 
-        my $qga = $src_conf->{agent}
+        my $qga = PVE::QemuServer::Agent::get_qga_key($src_conf, 'enabled')
             && (PVE::QemuServer::Agent::get_qga_key($src_conf, 'guest-fsfreeze') // 1);
 
         return PVE::QemuServer::clone_disk(
@@ -4562,7 +4562,7 @@ __PACKAGE__->register_method({
                     $dest_info->{efisize} = PVE::QemuServer::get_efivars_size($oldconf)
                         if $opt eq 'efidisk0';
 
-                    my $qga = $oldconf->{agent}
+                    my $qga = PVE::QemuServer::Agent::get_qga_key($oldconf, 'enabled')
                         && (PVE::QemuServer::Agent::get_qga_key($oldconf, 'guest-fsfreeze') // 1);
 
                     my $newdrive = PVE::QemuServer::clone_disk(
-- 
2.47.3

[PATCH qemu-server 3/8] backup: freeze: fix check if agent is enabled

[Fiona Ebner]

Just checking whether the configuration is present does not mean that
the guest agent is enabled. Explicitly check the 'enabled' property.
In case of clone/import and snapshot, this is already done. This
avoids a log message about the agent not running, since there is no
device added to the QEMU commandline for communicating with the agent
if disabled.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/VZDump/QemuServer.pm | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/src/PVE/VZDump/QemuServer.pm b/src/PVE/VZDump/QemuServer.pm
index 55fb6dc4..0681661d 100644
--- a/src/PVE/VZDump/QemuServer.pm
+++ b/src/PVE/VZDump/QemuServer.pm
@@ -1092,19 +1092,24 @@ sub _get_task_devlist {
 sub qga_fs_freeze {
     my ($self, $task, $vmid) = @_;
     return
-        if !$self->{vmlist}->{$vmid}->{agent}
-        || $task->{mode} eq 'stop'
+        if $task->{mode} eq 'stop'
         || !$self->{vm_was_running}
         || $self->{vm_was_paused};
 
+    my $conf = $self->{vmlist}->{$vmid};
+
+    if (!PVE::QemuServer::Agent::get_qga_key($conf, 'enabled')) {
+        $self->loginfo("skipping guest-agent 'fs-freeze', agent not configured in VM options");
+        return;
+    }
+
     if (!PVE::QemuServer::Agent::qga_check_running($vmid, 1)) {
         $self->loginfo("skipping guest-agent 'fs-freeze', agent configured but not running?");
         return;
     }
 
-    my $freeze = PVE::QemuServer::Agent::get_qga_key($self->{vmlist}->{$vmid}, 'guest-fsfreeze');
-    $freeze //=
-        PVE::QemuServer::Agent::get_qga_key($self->{vmlist}->{$vmid}, 'freeze-fs-on-backup');
+    my $freeze = PVE::QemuServer::Agent::get_qga_key($conf, 'guest-fsfreeze');
+    $freeze //= PVE::QemuServer::Agent::get_qga_key($conf, 'freeze-fs-on-backup');
     $freeze //= 1;
 
     if (!$freeze) {
-- 
2.47.3

[PATCH qemu-server 4/8] agent: parse: change signature to take property string rather than full VM config

[Fiona Ebner]

Better encapsulation and in preparation to harmonize checks for freeze
while avoiding the need to pass the full config to block jobs.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuServer.pm       | 6 +++---
 src/PVE/QemuServer/Agent.pm | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 545b653d..e94dae0d 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -3421,7 +3421,7 @@ sub config_to_command {
 
     push @$cmd, '-k', $conf->{keyboard} if defined($conf->{keyboard});
 
-    my $guest_agent = parse_guest_agent($conf);
+    my $guest_agent = parse_guest_agent($conf->{agent});
 
     if ($guest_agent->{enabled}) {
         my $qgasocket = PVE::QemuServer::Helpers::qmp_socket(
@@ -5144,8 +5144,8 @@ sub vmconfig_update_agent {
 
     my $hotplug_options = { fstrim_cloned_disks => 1 };
 
-    my $old_agent = parse_guest_agent($conf);
-    my $agent = parse_guest_agent({ $opt => $value });
+    my $old_agent = parse_guest_agent($conf->{agent});
+    my $agent = parse_guest_agent($value);
 
     for my $option (keys %$agent) { # added/changed options
         next if defined($hotplug_options->{$option});
diff --git a/src/PVE/QemuServer/Agent.pm b/src/PVE/QemuServer/Agent.pm
index 1e7b1895..974e282e 100644
--- a/src/PVE/QemuServer/Agent.pm
+++ b/src/PVE/QemuServer/Agent.pm
@@ -68,11 +68,11 @@ our $agent_fmt = {
 };
 
 sub parse_guest_agent {
-    my ($conf) = @_;
+    my ($value) = @_;
 
-    return {} if !defined($conf->{agent});
+    return {} if !defined($value);
 
-    my $res = eval { PVE::JSONSchema::parse_property_string($agent_fmt, $conf->{agent}) };
+    my $res = eval { PVE::JSONSchema::parse_property_string($agent_fmt, $value) };
     warn $@ if $@;
 
     # if the agent is disabled ignore the other potentially set properties
@@ -84,7 +84,7 @@ sub get_qga_key {
     my ($conf, $key) = @_;
     return undef if !defined($conf->{agent});
 
-    my $agent = parse_guest_agent($conf);
+    my $agent = parse_guest_agent($conf->{agent});
     return $agent->{$key};
 }
 
-- 
2.47.3

[PATCH qemu-server 5/8] agent: get qga key: change signature to take property string rather than full VM config

[Fiona Ebner]

Better encapsulation and in preparation to harmonize checks for freeze
while avoiding the need to pass the full config to block jobs.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/API2/Qemu.pm         | 17 +++++++++++------
 src/PVE/QemuConfig.pm        |  4 ++--
 src/PVE/QemuMigrate.pm       |  2 +-
 src/PVE/QemuServer.pm        |  6 +++++-
 src/PVE/QemuServer/Agent.pm  |  6 +++---
 src/PVE/VZDump/QemuServer.pm |  8 ++++----
 6 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index 1752f46a..841c4830 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -374,8 +374,8 @@ my $import_from_volid = sub {
 
         my ($src_storeid) = PVE::Storage::parse_volume_id($src_volid);
 
-        my $qga = PVE::QemuServer::Agent::get_qga_key($src_conf, 'enabled')
-            && (PVE::QemuServer::Agent::get_qga_key($src_conf, 'guest-fsfreeze') // 1);
+        my $qga = PVE::QemuServer::Agent::get_qga_key($src_conf->{agent}, 'enabled')
+            && (PVE::QemuServer::Agent::get_qga_key($src_conf->{agent}, 'guest-fsfreeze') // 1);
 
         return PVE::QemuServer::clone_disk(
             $storecfg,
@@ -3386,7 +3386,7 @@ __PACKAGE__->register_method({
             $status->{spice} = 1 if $spice;
             $status->{clipboard} = $vga->{clipboard};
         }
-        $status->{agent} = 1 if PVE::QemuServer::Agent::get_qga_key($conf, 'enabled');
+        $status->{agent} = 1 if PVE::QemuServer::Agent::get_qga_key($conf->{agent}, 'enabled');
 
         return $status;
     },
@@ -4562,8 +4562,12 @@ __PACKAGE__->register_method({
                     $dest_info->{efisize} = PVE::QemuServer::get_efivars_size($oldconf)
                         if $opt eq 'efidisk0';
 
-                    my $qga = PVE::QemuServer::Agent::get_qga_key($oldconf, 'enabled')
-                        && (PVE::QemuServer::Agent::get_qga_key($oldconf, 'guest-fsfreeze') // 1);
+                    my $qga = PVE::QemuServer::Agent::get_qga_key($oldconf->{agent}, 'enabled')
+                        && (
+                            PVE::QemuServer::Agent::get_qga_key(
+                                $oldconf->{agent}, 'guest-fsfreeze',
+                            ) // 1
+                        );
 
                     my $newdrive = PVE::QemuServer::clone_disk(
                         $storecfg,
@@ -4861,7 +4865,8 @@ __PACKAGE__->register_method({
 
                 PVE::QemuConfig->write_config($vmid, $conf);
 
-                my $do_trim = PVE::QemuServer::Agent::get_qga_key($conf, 'fstrim_cloned_disks');
+                my $do_trim =
+                    PVE::QemuServer::Agent::get_qga_key($conf->{agent}, 'fstrim_cloned_disks');
                 if ($running && $do_trim && PVE::QemuServer::Agent::qga_check_running($vmid)) {
                     eval { mon_cmd($vmid, "guest-fstrim") };
                 }
diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm
index 462d1d6d..7656134b 100644
--- a/src/PVE/QemuConfig.pm
+++ b/src/PVE/QemuConfig.pm
@@ -297,9 +297,9 @@ sub __snapshot_check_freeze_needed {
         return (
             $running,
             $running
-                && PVE::QemuServer::Agent::get_qga_key($config, 'enabled')
+                && PVE::QemuServer::Agent::get_qga_key($config->{agent}, 'enabled')
                 && PVE::QemuServer::Agent::qga_check_running($vmid)
-                && (PVE::QemuServer::Agent::get_qga_key($config, 'guest-fsfreeze') // 1),
+                && (PVE::QemuServer::Agent::get_qga_key($config->{agent}, 'guest-fsfreeze') // 1),
         );
     } else {
         return ($running, 0);
diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm
index f7ec3227..31070111 100644
--- a/src/PVE/QemuMigrate.pm
+++ b/src/PVE/QemuMigrate.pm
@@ -1752,7 +1752,7 @@ sub phase3_cleanup {
 
         if (
             $self->{storage_migration}
-            && PVE::QemuServer::Agent::get_qga_key($conf, 'fstrim_cloned_disks')
+            && PVE::QemuServer::Agent::get_qga_key($conf->{agent}, 'fstrim_cloned_disks')
             && $self->{running}
         ) {
             if (!$self->{vm_was_paused}) {
diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index e94dae0d..039b6a8c 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -6181,7 +6181,11 @@ sub _do_vm_stop {
 
     eval {
         if ($shutdown) {
-            if (defined($conf) && get_qga_key($conf, 'enabled') && qga_check_running($vmid)) {
+            if (
+                defined($conf)
+                && get_qga_key($conf->{agent}, 'enabled')
+                && qga_check_running($vmid)
+            ) {
                 mon_cmd($vmid, "guest-shutdown", timeout => $timeout);
             } else {
                 mon_cmd($vmid, "system_powerdown");
diff --git a/src/PVE/QemuServer/Agent.pm b/src/PVE/QemuServer/Agent.pm
index 974e282e..2c1f580d 100644
--- a/src/PVE/QemuServer/Agent.pm
+++ b/src/PVE/QemuServer/Agent.pm
@@ -81,10 +81,10 @@ sub parse_guest_agent {
 }
 
 sub get_qga_key {
-    my ($conf, $key) = @_;
-    return undef if !defined($conf->{agent});
+    my ($agent_str, $key) = @_;
+    return undef if !defined($agent_str);
 
-    my $agent = parse_guest_agent($conf->{agent});
+    my $agent = parse_guest_agent($agent_str);
     return $agent->{$key};
 }
 
diff --git a/src/PVE/VZDump/QemuServer.pm b/src/PVE/VZDump/QemuServer.pm
index 0681661d..ed63e3c2 100644
--- a/src/PVE/VZDump/QemuServer.pm
+++ b/src/PVE/VZDump/QemuServer.pm
@@ -1096,9 +1096,9 @@ sub qga_fs_freeze {
         || !$self->{vm_was_running}
         || $self->{vm_was_paused};
 
-    my $conf = $self->{vmlist}->{$vmid};
+    my $agent_str = $self->{vmlist}->{$vmid}->{agent};
 
-    if (!PVE::QemuServer::Agent::get_qga_key($conf, 'enabled')) {
+    if (!PVE::QemuServer::Agent::get_qga_key($agent_str, 'enabled')) {
         $self->loginfo("skipping guest-agent 'fs-freeze', agent not configured in VM options");
         return;
     }
@@ -1108,8 +1108,8 @@ sub qga_fs_freeze {
         return;
     }
 
-    my $freeze = PVE::QemuServer::Agent::get_qga_key($conf, 'guest-fsfreeze');
-    $freeze //= PVE::QemuServer::Agent::get_qga_key($conf, 'freeze-fs-on-backup');
+    my $freeze = PVE::QemuServer::Agent::get_qga_key($agent_str, 'guest-fsfreeze');
+    $freeze //= PVE::QemuServer::Agent::get_qga_key($agent_str, 'freeze-fs-on-backup');
     $freeze //= 1;
 
     if (!$freeze) {
-- 
2.47.3

[PATCH qemu-server 6/8] clone disk/block jobs: change signatures to take guest agent property string

[Fiona Ebner]

In preparation to harmonize checks for guest fs freeze.

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

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index 841c4830..2c338295 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -374,9 +374,6 @@ my $import_from_volid = sub {
 
         my ($src_storeid) = PVE::Storage::parse_volume_id($src_volid);
 
-        my $qga = PVE::QemuServer::Agent::get_qga_key($src_conf->{agent}, 'enabled')
-            && (PVE::QemuServer::Agent::get_qga_key($src_conf->{agent}, 'guest-fsfreeze') // 1);
-
         return PVE::QemuServer::clone_disk(
             $storecfg,
             $source_info,
@@ -385,7 +382,7 @@ my $import_from_volid = sub {
             $vollist,
             undef,
             undef,
-            $qga,
+            $src_conf->{agent},
             PVE::Storage::get_bandwidth_limit('clone', [$src_storeid, $dest_info->{storage}]),
         );
     };
@@ -4562,13 +4559,6 @@ __PACKAGE__->register_method({
                     $dest_info->{efisize} = PVE::QemuServer::get_efivars_size($oldconf)
                         if $opt eq 'efidisk0';
 
-                    my $qga = PVE::QemuServer::Agent::get_qga_key($oldconf->{agent}, 'enabled')
-                        && (
-                            PVE::QemuServer::Agent::get_qga_key(
-                                $oldconf->{agent}, 'guest-fsfreeze',
-                            ) // 1
-                        );
-
                     my $newdrive = PVE::QemuServer::clone_disk(
                         $storecfg,
                         $source_info,
@@ -4577,7 +4567,7 @@ __PACKAGE__->register_method({
                         $newvollist,
                         $jobs,
                         $completion,
-                        $qga,
+                        $oldconf->{agent},
                         $clonelimit,
                     );
 
diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 039b6a8c..5d900239 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -7939,7 +7939,7 @@ sub clone_disk {
             $dest_info->{'zero-initialized'} = 1 if $sparseinit;
             $dest_info->{vmid} = $newvmid if defined($newvmid);
             my $mirror_opts = {};
-            $mirror_opts->{'guest-agent'} = 1 if $qga;
+            $mirror_opts->{'guest-agent'} = $qga;
             $mirror_opts->{bwlimit} = $bwlimit if defined($bwlimit);
             PVE::QemuServer::BlockJob::mirror(
                 $source_info,
diff --git a/src/PVE/QemuServer/BlockJob.pm b/src/PVE/QemuServer/BlockJob.pm
index d7c78741..dc5804a6 100644
--- a/src/PVE/QemuServer/BlockJob.pm
+++ b/src/PVE/QemuServer/BlockJob.pm
@@ -162,13 +162,16 @@ sub monitor {
                 last if $completion eq 'skip' || $completion eq 'auto';
 
                 if ($vmiddst && $vmiddst != $vmid) {
-                    my $should_fsfreeze = $qga && qga_check_running($vmid);
+                    my $freeze_enabled =
+                        PVE::QemuServer::Agent::get_qga_key($qga, 'enabled') &&
+                        (PVE::QemuServer::Agent::get_qga_key($qga, 'guest-fsfreeze') // 1);
+                    my $should_fsfreeze = $freeze_enabled && qga_check_running($vmid);
                     if ($should_fsfreeze) {
                         print "issuing guest agent 'guest-fsfreeze-freeze' command\n";
                         eval { PVE::QemuServer::Agent::guest_fsfreeze($vmid); };
                         warn $@ if $@;
                     } else {
-                        if (!$qga) {
+                        if (!$freeze_enabled) {
                             print "skipping guest-agent 'guest-fsfreeze-freeze', disabled in VM"
                                 . " options\n";
                         } else {
@@ -430,8 +433,9 @@ original drive to use the new target.
 
 =over
 
-=item C<< $options->{'guest-agent'} >>: If the guest agent is configured for the VM. It will be used
-to freeze and thaw the filesystems for consistency when the target belongs to a different VM.
+=item C<< $options->{'guest-agent'} >>: The guest agent configuration of the VM, i.e. the 'agent'
+property string. If enabled by the configuration, freeze and thaw the filesystems for consistency
+when the target belongs to a different VM.
 
 =item C<< $options->{'bwlimit'} >>: The bandwidth limit to use for the mirroring operation, in
 KiB/s.
-- 
2.47.3

[PATCH qemu-server 7/8] agent: fsfreeze: harmonize checks for guest fs freeze

[Fiona Ebner]

Puts the logic inside the agent module, rather than duplicating it in
three different places.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuConfig.pm          | 14 ++++-------
 src/PVE/QemuServer/Agent.pm    | 45 ++++++++++++++++++++++++++++++++++
 src/PVE/QemuServer/BlockJob.pm | 13 ++--------
 src/PVE/VZDump/QemuServer.pm   | 21 ++++------------
 4 files changed, 57 insertions(+), 36 deletions(-)

diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm
index 7656134b..240b8bfd 100644
--- a/src/PVE/QemuConfig.pm
+++ b/src/PVE/QemuConfig.pm
@@ -292,18 +292,14 @@ sub __snapshot_check_running {
 sub __snapshot_check_freeze_needed {
     my ($class, $vmid, $config, $save_vmstate) = @_;
 
+    my $freeze_needed = 0;
     my $running = $class->__snapshot_check_running($vmid);
+
     if (!$save_vmstate) {
-        return (
-            $running,
-            $running
-                && PVE::QemuServer::Agent::get_qga_key($config->{agent}, 'enabled')
-                && PVE::QemuServer::Agent::qga_check_running($vmid)
-                && (PVE::QemuServer::Agent::get_qga_key($config->{agent}, 'guest-fsfreeze') // 1),
-        );
-    } else {
-        return ($running, 0);
+        $freeze_needed = PVE::QemuServer::Agent::guest_fsfreeze_applicable($config->{agent}, $vmid);
     }
+
+    return ($running, $freeze_needed);
 }
 
 sub __snapshot_freeze {
diff --git a/src/PVE/QemuServer/Agent.pm b/src/PVE/QemuServer/Agent.pm
index 2c1f580d..12f9f9d5 100644
--- a/src/PVE/QemuServer/Agent.pm
+++ b/src/PVE/QemuServer/Agent.pm
@@ -289,4 +289,49 @@ sub guest_fsthaw {
     return;
 }
 
+=head3 guest_fsfreeze_applicable
+
+    if (guest_fsfreeze_applicable($agent_str, $vmid, $logfunc, $is_backup)) {
+        guest_fsfreeze($vmid);
+    }
+
+Check if the file systems of the guest C<$vmid> should be frozen according to the guest agent
+property string C<$agent_str> and if freezing is actionable in practice, i.e. guest agent running.
+Logs a message if skipped. Using a custom log function via C<$logfunc> is supported. Otherwise,
+C<print()> and C<warn()> will be used. In the context of backup tasks, C<$is_backup> must be
+specified.
+
+=cut
+
+sub guest_fsfreeze_applicable {
+    my ($agent_str, $vmid, $logfunc, $is_backup) = @_;
+
+    $logfunc //= sub {
+        my ($level, $msg) = @_;
+        chomp($msg);
+        $level eq 'info' ? print("$msg\n") : warn("$msg\n");
+    };
+
+    if (!get_qga_key($agent_str, 'enabled')) {
+        $logfunc->('info', "skipping guest filesystem freeze - agent not configured in VM options");
+        return;
+    }
+
+    if (!qga_check_running($vmid, 1)) {
+        $logfunc->('info', "skipping guest filesystem freeze - agent configured but not running?");
+        return;
+    }
+
+    my $freeze = get_qga_key($agent_str, 'guest-fsfreeze');
+    $freeze //= get_qga_key($agent_str, 'freeze-fs-on-backup') if $is_backup;
+    $freeze //= 1;
+
+    if (!$freeze) {
+        $logfunc->('info', "skipping guest filesystem freeze - disabled in VM options");
+        return;
+    }
+
+    return 1;
+}
+
 1;
diff --git a/src/PVE/QemuServer/BlockJob.pm b/src/PVE/QemuServer/BlockJob.pm
index dc5804a6..35ebbd0e 100644
--- a/src/PVE/QemuServer/BlockJob.pm
+++ b/src/PVE/QemuServer/BlockJob.pm
@@ -162,22 +162,13 @@ sub monitor {
                 last if $completion eq 'skip' || $completion eq 'auto';
 
                 if ($vmiddst && $vmiddst != $vmid) {
-                    my $freeze_enabled =
-                        PVE::QemuServer::Agent::get_qga_key($qga, 'enabled') &&
-                        (PVE::QemuServer::Agent::get_qga_key($qga, 'guest-fsfreeze') // 1);
-                    my $should_fsfreeze = $freeze_enabled && qga_check_running($vmid);
+                    my $should_fsfreeze =
+                        PVE::QemuServer::Agent::guest_fsfreeze_applicable($qga, $vmid);
                     if ($should_fsfreeze) {
                         print "issuing guest agent 'guest-fsfreeze-freeze' command\n";
                         eval { PVE::QemuServer::Agent::guest_fsfreeze($vmid); };
                         warn $@ if $@;
                     } else {
-                        if (!$freeze_enabled) {
-                            print "skipping guest-agent 'guest-fsfreeze-freeze', disabled in VM"
-                                . " options\n";
-                        } else {
-                            print "skipping guest agent 'guest-fsfreeze-freeze' command: the"
-                                . " agent is not running inside of the guest\n";
-                        }
                         print "suspend vm\n";
                         eval { PVE::QemuServer::RunState::vm_suspend($vmid, 1); };
                         warn $@ if $@;
diff --git a/src/PVE/VZDump/QemuServer.pm b/src/PVE/VZDump/QemuServer.pm
index ed63e3c2..99b31dd9 100644
--- a/src/PVE/VZDump/QemuServer.pm
+++ b/src/PVE/VZDump/QemuServer.pm
@@ -1098,22 +1098,11 @@ sub qga_fs_freeze {
 
     my $agent_str = $self->{vmlist}->{$vmid}->{agent};
 
-    if (!PVE::QemuServer::Agent::get_qga_key($agent_str, 'enabled')) {
-        $self->loginfo("skipping guest-agent 'fs-freeze', agent not configured in VM options");
-        return;
-    }
-
-    if (!PVE::QemuServer::Agent::qga_check_running($vmid, 1)) {
-        $self->loginfo("skipping guest-agent 'fs-freeze', agent configured but not running?");
-        return;
-    }
-
-    my $freeze = PVE::QemuServer::Agent::get_qga_key($agent_str, 'guest-fsfreeze');
-    $freeze //= PVE::QemuServer::Agent::get_qga_key($agent_str, 'freeze-fs-on-backup');
-    $freeze //= 1;
-
-    if (!$freeze) {
-        $self->loginfo("skipping guest-agent 'fs-freeze', disabled in VM options");
+    my $logfunc = sub {
+        my ($level, $msg) = @_;
+        $self->log($level, $msg);
+    };
+    if (!PVE::QemuServer::Agent::guest_fsfreeze_applicable($agent_str, $vmid, $logfunc, 1)) {
         return;
     }
 
-- 
2.47.3

[PATCH qemu-server 8/8] fix #7383: agent: fsfreeze: skip freeze if already frozen

[Fiona Ebner]

This avoids the following error message about the command being
disallowed which can be hard to interpret for users:
> Command guest-fsfreeze-freeze has been disabled: the command is not allowed

It's disallowed, because the fs is already frozen, but the error
message does not indicate this. Instead avoid the error by skipping if
already frozen and log it if that is the case.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuServer/Agent.pm | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/src/PVE/QemuServer/Agent.pm b/src/PVE/QemuServer/Agent.pm
index 12f9f9d5..54e30bcf 100644
--- a/src/PVE/QemuServer/Agent.pm
+++ b/src/PVE/QemuServer/Agent.pm
@@ -289,6 +289,25 @@ sub guest_fsthaw {
     return;
 }
 
+=head3 guest_fs_is_frozen
+
+    if (guest_fs_is_frozen($vmid)) {
+        print "skipping guest fs freeze - already frozen\n";
+    }
+
+Check if the file systems of the guest C<$vmid> is currently frozen.
+
+=cut
+
+sub guest_fs_is_frozen {
+    my ($vmid) = @_;
+
+    my $status = PVE::QemuServer::Monitor::mon_cmd($vmid, "guest-fsfreeze-status");
+    check_agent_error($status, "unable to check guest filesystem freeze status");
+
+    return $status eq 'frozen';
+}
+
 =head3 guest_fsfreeze_applicable
 
     if (guest_fsfreeze_applicable($agent_str, $vmid, $logfunc, $is_backup)) {
@@ -296,10 +315,10 @@ sub guest_fsthaw {
     }
 
 Check if the file systems of the guest C<$vmid> should be frozen according to the guest agent
-property string C<$agent_str> and if freezing is actionable in practice, i.e. guest agent running.
-Logs a message if skipped. Using a custom log function via C<$logfunc> is supported. Otherwise,
-C<print()> and C<warn()> will be used. In the context of backup tasks, C<$is_backup> must be
-specified.
+property string C<$agent_str> and if freezing is actionable in practice, i.e. guest agent running
+and file system not already frozen. Logs a message if skipped. Using a custom log function via
+C<$logfunc> is supported. Otherwise, C<print()> and C<warn()> will be used. In the context of backup
+tasks, C<$is_backup> must be specified.
 
 =cut
 
@@ -331,6 +350,14 @@ sub guest_fsfreeze_applicable {
         return;
     }
 
+    my $frozen = eval { PVE::QemuServer::Agent::guest_fs_is_frozen($vmid) };
+    $logfunc->('warn', "unable to check guest fs freeze status - $@") if $@;
+
+    if ($frozen) {
+        $logfunc->('info', "skipping guest filesystem freeze - already frozen");
+        return;
+    }
+
     return 1;
 }
 
-- 
2.47.3

Re: [PATCH qemu-server 7/8] agent: fsfreeze: harmonize checks for guest fs freeze

[Thomas Lamprecht]

Am 24.03.26 um 14:53 schrieb Fiona Ebner:
> Puts the logic inside the agent module, rather than duplicating it in
> three different places.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/QemuConfig.pm          | 14 ++++-------
>  src/PVE/QemuServer/Agent.pm    | 45 ++++++++++++++++++++++++++++++++++
>  src/PVE/QemuServer/BlockJob.pm | 13 ++--------
>  src/PVE/VZDump/QemuServer.pm   | 21 ++++------------
>  4 files changed, 57 insertions(+), 36 deletions(-)
> 
> diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm
> index 7656134b..240b8bfd 100644
> --- a/src/PVE/QemuConfig.pm
> +++ b/src/PVE/QemuConfig.pm
> @@ -292,18 +292,14 @@ sub __snapshot_check_running {
>  sub __snapshot_check_freeze_needed {
>      my ($class, $vmid, $config, $save_vmstate) = @_;
>  
> +    my $freeze_needed = 0;
>      my $running = $class->__snapshot_check_running($vmid);
> +
>      if (!$save_vmstate) {
> -        return (
> -            $running,
> -            $running
> -                && PVE::QemuServer::Agent::get_qga_key($config->{agent}, 'enabled')
> -                && PVE::QemuServer::Agent::qga_check_running($vmid)
> -                && (PVE::QemuServer::Agent::get_qga_key($config->{agent}, 'guest-fsfreeze') // 1),
> -        );
> -    } else {
> -        return ($running, 0);
> +        $freeze_needed = PVE::QemuServer::Agent::guest_fsfreeze_applicable($config->{agent}, $vmid);
>      }
> +
> +    return ($running, $freeze_needed);
>  }
>  
>  sub __snapshot_freeze {
> diff --git a/src/PVE/QemuServer/Agent.pm b/src/PVE/QemuServer/Agent.pm
> index 2c1f580d..12f9f9d5 100644
> --- a/src/PVE/QemuServer/Agent.pm
> +++ b/src/PVE/QemuServer/Agent.pm
> @@ -289,4 +289,49 @@ sub guest_fsthaw {
>      return;
>  }
>  
> +=head3 guest_fsfreeze_applicable
> +
> +    if (guest_fsfreeze_applicable($agent_str, $vmid, $logfunc, $is_backup)) {
> +        guest_fsfreeze($vmid);
> +    }
> +
> +Check if the file systems of the guest C<$vmid> should be frozen according to the guest agent
> +property string C<$agent_str> and if freezing is actionable in practice, i.e. guest agent running.
> +Logs a message if skipped. Using a custom log function via C<$logfunc> is supported. Otherwise,
> +C<print()> and C<warn()> will be used. In the context of backup tasks, C<$is_backup> must be
> +specified.
> +
> +=cut
> +
> +sub guest_fsfreeze_applicable {
> +    my ($agent_str, $vmid, $logfunc, $is_backup) = @_;

I'm a bit torn here, on one hand I can see where this comes from, but OTOH it
also feels a bit overly opaque. Nicest gain here is probably the centralized
log method, which I can see the benefits for. So probably fine to have...

> +
> +    $logfunc //= sub {
> +        my ($level, $msg) = @_;
> +        chomp($msg);
> +        $level eq 'info' ? print("$msg\n") : warn("$msg\n");

There's a single use of a level other than 'info' in the next patch and the
chomp feels superfluous considering this is a local closure and we fully
control the messages and their trailing newlines here?

And we probably should start to encourage use v5.36+signature migrations
for (smaller) modules soonish, as then we would be able to use (untested): 

$logfunc //= sub ($msg, $level = 'info') { 
    $level eq 'info' ? print("$msg\n") : warn("$msg\n");
}

> +    };
> +
> +    if (!get_qga_key($agent_str, 'enabled')) {
> +        $logfunc->('info', "skipping guest filesystem freeze - agent not configured in VM options");
> +        return;
> +    }
> +
> +    if (!qga_check_running($vmid, 1)) {
> +        $logfunc->('info', "skipping guest filesystem freeze - agent configured but not running?");
> +        return;
> +    }
> +
> +    my $freeze = get_qga_key($agent_str, 'guest-fsfreeze');
> +    $freeze //= get_qga_key($agent_str, 'freeze-fs-on-backup') if $is_backup;

$is_backup could be dropped when aliasing these to mean the same, if
something akin to my series [0] gets applied.

[0]: https://lore.proxmox.com/all/20260325210021.3789748-1-t.lamprecht@proxmox.com/T/#m837483e3ee5f17e5949485f11c7965367c716b03

> +    $freeze //= 1;
> +
> +    if (!$freeze) {
> +        $logfunc->('info', "skipping guest filesystem freeze - disabled in VM options");
> +        return;
> +    }
> +
> +    return 1;
> +}
> +
>  1;

applied: [PATCH qemu-server 1/8] agent: fs thaw: check command result

[Thomas Lamprecht]

On Tue, 24 Mar 2026 14:50:41 +0100, Fiona Ebner wrote:
> All callers already use eval{} and warn, so this does not change the
> execution flow for any callers, but thaw errors will not be quietly
> ignored anymore.
> 
> 

Applied, thanks!

[1/8] agent: fs thaw: check command result
      commit: adc880910bb8ecb0ed82a9a79100c0e7896b4d3a

applied: [PATCH qemu-server 2/8] api: clone/import: fix check if agent is enabled

[Thomas Lamprecht]

On Tue, 24 Mar 2026 14:50:42 +0100, Fiona Ebner wrote:
> Just checking whether the configuration is present does not mean that
> the guest agent is enabled. Explicitly check the 'enabled' property.
> In case of snapshot, this is already done. This avoids a log message
> about the agent not running, since there is no device added to the
> QEMU commandline for communicating with the agent if disabled.
> 
> 
> [...]

Applied, thanks!

[2/8] api: clone/import: fix check if agent is enabled
      commit: ee60a56e331d531673803155ce7b0c6437676107

applied: [PATCH qemu-server 3/8] backup: freeze: fix check if agent is enabled

[Thomas Lamprecht]

On Tue, 24 Mar 2026 14:50:43 +0100, Fiona Ebner wrote:
> Just checking whether the configuration is present does not mean that
> the guest agent is enabled. Explicitly check the 'enabled' property.
> In case of clone/import and snapshot, this is already done. This
> avoids a log message about the agent not running, since there is no
> device added to the QEMU commandline for communicating with the agent
> if disabled.
> 
> [...]

Applied, thanks!

[3/8] backup: freeze: fix check if agent is enabled
      commit: 990e6b805e793a6650febfb587c9c7fde43ba00c