public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [PATCH qemu-server v2 0/2] fix #7828: avoid parsing absolute paths as volume ids.
@ 2026-07-22  9:52 Thomas Ellmenreich
  2026-07-22  9:52 ` [PATCH qemu-server v2 1/2] refactor checks for absolute path as drive Thomas Ellmenreich
  2026-07-22  9:52 ` [PATCH qemu-server v2 2/2] fix #7828: avoid parsing absolute paths as volume ids Thomas Ellmenreich
  0 siblings, 2 replies; 9+ messages in thread
From: Thomas Ellmenreich @ 2026-07-22  9:52 UTC (permalink / raw)
  To: pve-devel; +Cc: Thomas Ellmenreich

This series refactors all checks of drive and volume ids as absolute paths
into two separate helper functions, and uses them in all occurrences of such
checks.

The new helper functions are then also applied to make sure that drives that
are absolute paths are not added as unused drives to a vm. Just like is the
case for cdrom drives.

This issue was reported in the community forum: [0]. The issue arose, as
@Daniel Kral mentioned, because of the addition of the config check and parsing
of the volume id with commit: 5b5c5768, which did not consider the possibility
of absolute paths as volume ids.

Changes since v1:

- Prepended an additional patch to introduce the two helper functions, as the
  check for absolute paths was performed a considerable amount of times.

- Use of the created helper functions:

- Addition of the "Fixes 5b..." to the commit message

[0]: https://forum.proxmox.com/threads/possible-regression-in-pve-9-2-x-unable-to-remove-physical-passthrough-disks-using-qm-set-delete.185191/


qemu-manager:

Thomas Ellmenreich (2):
  refactor checks for absolute path as drive
  fix #7828: avoid parsing absolute paths as volume ids

 src/PVE/QemuConfig.pm          |  2 +-
 src/PVE/QemuMigrate.pm         |  2 +-
 src/PVE/QemuServer.pm          | 22 +++++++++-------------
 src/PVE/QemuServer/Blockdev.pm |  4 ++--
 src/PVE/QemuServer/Drive.pm    |  8 +++++++-
 src/PVE/QemuServer/Helpers.pm  |  5 +++++
 6 files changed, 25 insertions(+), 18 deletions(-)


Summary over all repositories:
  6 files changed, 25 insertions(+), 18 deletions(-)

-- 
Generated by murpp 0.12.0




^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH qemu-server v2 1/2] refactor checks for absolute path as drive
  2026-07-22  9:52 [PATCH qemu-server v2 0/2] fix #7828: avoid parsing absolute paths as volume ids Thomas Ellmenreich
@ 2026-07-22  9:52 ` Thomas Ellmenreich
  2026-07-22 12:45   ` Daniel Kral
  2026-07-22  9:52 ` [PATCH qemu-server v2 2/2] fix #7828: avoid parsing absolute paths as volume ids Thomas Ellmenreich
  1 sibling, 1 reply; 9+ messages in thread
From: Thomas Ellmenreich @ 2026-07-22  9:52 UTC (permalink / raw)
  To: pve-devel; +Cc: Thomas Ellmenreich

Previously there were a bunch of separate checks for whether a drive was
an absolute path. These have been refactored to use two checks in
QemuServer::Drive and QemuServer::Helpers.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 src/PVE/QemuConfig.pm          |  2 +-
 src/PVE/QemuMigrate.pm         |  2 +-
 src/PVE/QemuServer.pm          | 20 ++++++++------------
 src/PVE/QemuServer/Blockdev.pm |  4 ++--
 src/PVE/QemuServer/Drive.pm    |  8 +++++++-
 src/PVE/QemuServer/Helpers.pm  |  5 +++++
 6 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm
index 26f0fda2..d8733d9d 100644
--- a/src/PVE/QemuConfig.pm
+++ b/src/PVE/QemuConfig.pm
@@ -145,7 +145,7 @@ sub get_replicatable_volumes {
 
         return if !$cleanup && !$attr->{replicate};
 
-        if ($volid =~ m|^/|) {
+        if (PVE::QemuServer::Helpers::is_absolute_path($volid)) {
             return if !$attr->{replicate};
             return if $cleanup || $noerr;
             die "unable to replicate local file/device '$volid'\n";
diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm
index 8da6f15d..dcf911a5 100644
--- a/src/PVE/QemuMigrate.pm
+++ b/src/PVE/QemuMigrate.pm
@@ -405,7 +405,7 @@ sub scan_local_volumes {
         my $test_volid = sub {
             my ($volid, $attr) = @_;
 
-            if ($volid =~ m|^/|) {
+            if (PVE::QemuServer::Helpers::is_absolute_path($volid)) {
                 return if $attr->{shared};
                 $local_volumes->{$volid}->{ref} = 'config';
                 die "local file/device\n";
diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 191ae549..ce832c8e 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -75,6 +75,7 @@ use PVE::QemuServer::Drive qw(
     checked_volume_format
     drive_is_cloudinit
     drive_is_cdrom
+    drive_is_absolute_path
     parse_drive
     print_drive
     storage_allows_io_uring_default
@@ -913,7 +914,7 @@ PVE::JSONSchema::register_format(
 sub verify_volume_id_or_absolute_path {
     my ($volid, $noerr) = @_;
 
-    return $volid if $volid =~ m|^/|;
+    return $volid if PVE::QemuServer::Helpers::is_absolute_path($volid);
 
     $volid = eval { PVE::JSONSchema::check_format('pve-volume-id', $volid, '') };
     if ($@) {
@@ -1555,7 +1556,7 @@ sub print_vga_device {
 sub vm_is_volid_owner {
     my ($storecfg, $vmid, $volid) = @_;
 
-    if ($volid !~ m|^/|) {
+    if (!PVE::QemuServer::Helpers::is_absolute_path($volid)) {
         my ($path, $owner);
         eval { ($path, $owner) = PVE::Storage::path($storecfg, $volid); };
         log_warn("ownership of volume '$volid' could not be determined: $@") if $@;
@@ -1841,11 +1842,9 @@ sub destroy_vm {
             { include_unused => 1 },
             sub {
                 my ($ds, $drive) = @_;
-                return if drive_is_cdrom($drive);
+                return if drive_is_cdrom($drive) || drive_is_absolute_path($drive);
 
                 my $volid = $drive->{file};
-                return if !$volid || $volid =~ m|^/|;
-
                 my $result = eval { PVE::Storage::volume_is_base_and_used($storecfg, $volid) };
                 # early check, removal of volume will fail later anyway, so warning here is fine
                 log_warn("failed to check if volume '$volid' is used by linked clones: $@")
@@ -1859,10 +1858,9 @@ sub destroy_vm {
     my $volids = {};
     my $remove_owned_drive = sub {
         my ($ds, $drive) = @_;
-        return if drive_is_cdrom($drive, 1);
+        return if drive_is_cdrom($drive, 1) || drive_is_absolute_path($drive);
 
         my $volid = $drive->{file};
-        return if !$volid || $volid =~ m|^/|;
         return if $volids->{$volid};
 
         my ($path, $owner) = eval { PVE::Storage::path($storecfg, $volid) };
@@ -6085,7 +6083,7 @@ sub get_vm_volumes {
         sub {
             my ($volid, $attr) = @_;
 
-            return if $volid =~ m|^/|;
+            return if PVE::QemuServer::Helpers::is_absolute_path($volid);
 
             my ($sid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
             return if !$sid;
@@ -6485,7 +6483,7 @@ sub tar_restore_cleanup {
             if ($line =~ m/vzdump:([^\s:]*):(\S+)$/) {
                 my $volid = $2;
                 eval {
-                    if ($volid =~ m|^/|) {
+                    if (PVE::QemuServer::Helpers::is_absolute_path($volid)) {
                         unlink $volid || die 'unlink failed\n';
                     } else {
                         PVE::Storage::vdisk_free($storecfg, $volid);
@@ -6530,11 +6528,9 @@ my $restore_cleanup_oldconf = sub {
         sub {
             my ($ds, $drive) = @_;
 
-            return if drive_is_cdrom($drive, 1);
+            return if drive_is_cdrom($drive, 1) || drive_is_absolute_path($drive);
 
             my $volid = $drive->{file};
-            return if !$volid || $volid =~ m|^/|;
-
             my ($path, $owner) = PVE::Storage::path($storecfg, $volid);
             return if !$path || !$owner || ($owner != $vmid);
 
diff --git a/src/PVE/QemuServer/Blockdev.pm b/src/PVE/QemuServer/Blockdev.pm
index 101c747c..78a53ff5 100644
--- a/src/PVE/QemuServer/Blockdev.pm
+++ b/src/PVE/QemuServer/Blockdev.pm
@@ -11,7 +11,7 @@ use JSON;
 use PVE::JSONSchema qw(json_bool);
 use PVE::Storage;
 
-use PVE::QemuServer::Drive qw(drive_is_cdrom);
+use PVE::QemuServer::Drive qw(drive_is_cdrom drive_is_absolute_path);
 use PVE::QemuServer::Helpers;
 use PVE::QemuServer::Machine;
 use PVE::QemuServer::Monitor qw(mon_cmd qmp_cmd qsd_qmp_peer vm_qmp_peer);
@@ -299,7 +299,7 @@ sub generate_file_blockdev {
     } elsif ($drive->{file} eq 'cdrom') {
         my $path = PVE::QemuServer::Drive::get_iso_path($storecfg, $drive->{file});
         $blockdev = { driver => 'host_cdrom', filename => "$path" };
-    } elsif ($drive->{file} =~ m|^/|) {
+    } elsif (drive_is_absolute_path($drive)) {
         my $path = $drive->{file};
         # The 'file' driver only works for regular files. The check below is taken from
         # block/file-posix.c:hdev_probe_device() in QEMU. To detect CD-ROM host devices, QEMU issues
diff --git a/src/PVE/QemuServer/Drive.pm b/src/PVE/QemuServer/Drive.pm
index b80b7dbb..35256acf 100644
--- a/src/PVE/QemuServer/Drive.pm
+++ b/src/PVE/QemuServer/Drive.pm
@@ -23,6 +23,7 @@ our @EXPORT_OK = qw(
     checked_volume_format
     drive_is_cloudinit
     drive_is_cdrom
+    drive_is_absolute_path
     parse_drive
     print_drive
     storage_allows_io_uring_default
@@ -766,6 +767,11 @@ sub drive_is_cdrom {
     return $drive && $drive->{media} && ($drive->{media} eq 'cdrom');
 }
 
+sub drive_is_absolute_path {
+    my ($drive) = @_;
+    return PVE::QemuServer::Helpers::is_absolute_path($drive->{file});
+}
+
 sub parse_drive_interface {
     my ($key) = @_;
 
@@ -1061,7 +1067,7 @@ sub get_scsi_device_type {
     if (drive_is_cdrom($drive) || drive_is_cloudinit($drive)) {
         $devicetype = 'cd';
     } else {
-        if ($drive->{file} =~ m|^/|) {
+        if (drive_is_absolute_path($drive)) {
             $path = $drive->{file};
             if (my $info = path_is_scsi($path)) {
                 if ($info->{type} == 0 && $drive->{scsiblock}) {
diff --git a/src/PVE/QemuServer/Helpers.pm b/src/PVE/QemuServer/Helpers.pm
index dd17eef5..5cc3e8b5 100644
--- a/src/PVE/QemuServer/Helpers.pm
+++ b/src/PVE/QemuServer/Helpers.pm
@@ -389,4 +389,9 @@ sub get_host_phys_address_bits {
     return; # undef, cannot really do anything..
 }
 
+sub is_absolute_path {
+    my ($volid) = @_;
+    return $volid =~ m|^/|;
+}
+
 1;
-- 
2.47.3





^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH qemu-server v2 2/2] fix #7828: avoid parsing absolute paths as volume ids
  2026-07-22  9:52 [PATCH qemu-server v2 0/2] fix #7828: avoid parsing absolute paths as volume ids Thomas Ellmenreich
  2026-07-22  9:52 ` [PATCH qemu-server v2 1/2] refactor checks for absolute path as drive Thomas Ellmenreich
@ 2026-07-22  9:52 ` Thomas Ellmenreich
  2026-07-22 13:01   ` Fiona Ebner
  2026-07-22 14:10   ` Elias Huhsovitz
  1 sibling, 2 replies; 9+ messages in thread
From: Thomas Ellmenreich @ 2026-07-22  9:52 UTC (permalink / raw)
  To: pve-devel; +Cc: Thomas Ellmenreich

Just like in the case of cdroms, absolute paths to storage drives can be
skipped when beeing marked as unused as they cannot be owned by the vm.

Fixes: 5b5c5768 ("display warnings for storage errors or if storage no longer exists")
Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 src/PVE/QemuServer.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index ce832c8e..ce0eda74 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -1575,7 +1575,7 @@ sub vmconfig_register_unused_drive {
         eval { PVE::Storage::vdisk_free($storecfg, $drive->{file}) };
         warn $@ if $@;
         delete $conf->{'special-sections'}->{cloudinit};
-    } elsif (!drive_is_cdrom($drive)) {
+    } elsif (!drive_is_cdrom($drive) && !drive_is_absolute_path($drive)) {
         my $volid = $drive->{file};
         my ($storeid, undef) = PVE::Storage::parse_volume_id($volid);
         if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {
-- 
2.47.3





^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH qemu-server v2 1/2] refactor checks for absolute path as drive
  2026-07-22  9:52 ` [PATCH qemu-server v2 1/2] refactor checks for absolute path as drive Thomas Ellmenreich
@ 2026-07-22 12:45   ` Daniel Kral
  2026-07-22 13:40     ` Thomas Ellmenreich
  2026-07-22 14:01     ` Fiona Ebner
  0 siblings, 2 replies; 9+ messages in thread
From: Daniel Kral @ 2026-07-22 12:45 UTC (permalink / raw)
  To: Thomas Ellmenreich, pve-devel

Thanks for the quick v2!

On Wed Jul 22, 2026 at 11:52 AM CEST, Thomas Ellmenreich wrote:
> Previously there were a bunch of separate checks for whether a drive was
> an absolute path. These have been refactored to use two checks in
> QemuServer::Drive and QemuServer::Helpers.
>
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  src/PVE/QemuConfig.pm          |  2 +-
>  src/PVE/QemuMigrate.pm         |  2 +-
>  src/PVE/QemuServer.pm          | 20 ++++++++------------
>  src/PVE/QemuServer/Blockdev.pm |  4 ++--
>  src/PVE/QemuServer/Drive.pm    |  8 +++++++-
>  src/PVE/QemuServer/Helpers.pm  |  5 +++++
>  6 files changed, 24 insertions(+), 17 deletions(-)
>

[ snip ]

> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 191ae549..ce832c8e 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -75,6 +75,7 @@ use PVE::QemuServer::Drive qw(
>      checked_volume_format
>      drive_is_cloudinit
>      drive_is_cdrom
> +    drive_is_absolute_path

nit: should be sorted alphabetically

>      parse_drive
>      print_drive
>      storage_allows_io_uring_default
> @@ -913,7 +914,7 @@ PVE::JSONSchema::register_format(
>  sub verify_volume_id_or_absolute_path {
>      my ($volid, $noerr) = @_;
>  
> -    return $volid if $volid =~ m|^/|;
> +    return $volid if PVE::QemuServer::Helpers::is_absolute_path($volid);
>  
>      $volid = eval { PVE::JSONSchema::check_format('pve-volume-id', $volid, '') };
>      if ($@) {
> @@ -1555,7 +1556,7 @@ sub print_vga_device {
>  sub vm_is_volid_owner {
>      my ($storecfg, $vmid, $volid) = @_;
>  
> -    if ($volid !~ m|^/|) {
> +    if (!PVE::QemuServer::Helpers::is_absolute_path($volid)) {
>          my ($path, $owner);
>          eval { ($path, $owner) = PVE::Storage::path($storecfg, $volid); };
>          log_warn("ownership of volume '$volid' could not be determined: $@") if $@;
> @@ -1841,11 +1842,9 @@ sub destroy_vm {
>              { include_unused => 1 },
>              sub {
>                  my ($ds, $drive) = @_;
> -                return if drive_is_cdrom($drive);
> +                return if drive_is_cdrom($drive) || drive_is_absolute_path($drive);
>  
>                  my $volid = $drive->{file};
> -                return if !$volid || $volid =~ m|^/|;

Hm, the !$volid check should be included in the check above as well as
drive_is_absolute_path() doesn't check this and shouldn't need to.

Though if there's good reason that !$volid will never be true here, this
can be removed in a previous commit with a clear reasoning why.

> -
>                  my $result = eval { PVE::Storage::volume_is_base_and_used($storecfg, $volid) };
>                  # early check, removal of volume will fail later anyway, so warning here is fine
>                  log_warn("failed to check if volume '$volid' is used by linked clones: $@")
> @@ -1859,10 +1858,9 @@ sub destroy_vm {
>      my $volids = {};
>      my $remove_owned_drive = sub {
>          my ($ds, $drive) = @_;
> -        return if drive_is_cdrom($drive, 1);
> +        return if drive_is_cdrom($drive, 1) || drive_is_absolute_path($drive);
>  
>          my $volid = $drive->{file};
> -        return if !$volid || $volid =~ m|^/|;
>          return if $volids->{$volid};

same here

>  
>          my ($path, $owner) = eval { PVE::Storage::path($storecfg, $volid) };
> @@ -6085,7 +6083,7 @@ sub get_vm_volumes {
>          sub {
>              my ($volid, $attr) = @_;
>  
> -            return if $volid =~ m|^/|;
> +            return if PVE::QemuServer::Helpers::is_absolute_path($volid);
>  
>              my ($sid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
>              return if !$sid;
> @@ -6485,7 +6483,7 @@ sub tar_restore_cleanup {
>              if ($line =~ m/vzdump:([^\s:]*):(\S+)$/) {
>                  my $volid = $2;
>                  eval {
> -                    if ($volid =~ m|^/|) {
> +                    if (PVE::QemuServer::Helpers::is_absolute_path($volid)) {
>                          unlink $volid || die 'unlink failed\n';
>                      } else {
>                          PVE::Storage::vdisk_free($storecfg, $volid);
> @@ -6530,11 +6528,9 @@ my $restore_cleanup_oldconf = sub {
>          sub {
>              my ($ds, $drive) = @_;
>  
> -            return if drive_is_cdrom($drive, 1);
> +            return if drive_is_cdrom($drive, 1) || drive_is_absolute_path($drive);
>  
>              my $volid = $drive->{file};
> -            return if !$volid || $volid =~ m|^/|;

same here

> -
>              my ($path, $owner) = PVE::Storage::path($storecfg, $volid);
>              return if !$path || !$owner || ($owner != $vmid);
>  
> diff --git a/src/PVE/QemuServer/Blockdev.pm b/src/PVE/QemuServer/Blockdev.pm
> index 101c747c..78a53ff5 100644
> --- a/src/PVE/QemuServer/Blockdev.pm
> +++ b/src/PVE/QemuServer/Blockdev.pm
> @@ -11,7 +11,7 @@ use JSON;
>  use PVE::JSONSchema qw(json_bool);
>  use PVE::Storage;
>  
> -use PVE::QemuServer::Drive qw(drive_is_cdrom);
> +use PVE::QemuServer::Drive qw(drive_is_cdrom drive_is_absolute_path);

nit: should also be alphabetically sorted

>  use PVE::QemuServer::Helpers;
>  use PVE::QemuServer::Machine;
>  use PVE::QemuServer::Monitor qw(mon_cmd qmp_cmd qsd_qmp_peer vm_qmp_peer);
> @@ -299,7 +299,7 @@ sub generate_file_blockdev {
>      } elsif ($drive->{file} eq 'cdrom') {
>          my $path = PVE::QemuServer::Drive::get_iso_path($storecfg, $drive->{file});
>          $blockdev = { driver => 'host_cdrom', filename => "$path" };
> -    } elsif ($drive->{file} =~ m|^/|) {
> +    } elsif (drive_is_absolute_path($drive)) {
>          my $path = $drive->{file};
>          # The 'file' driver only works for regular files. The check below is taken from
>          # block/file-posix.c:hdev_probe_device() in QEMU. To detect CD-ROM host devices, QEMU issues
> diff --git a/src/PVE/QemuServer/Drive.pm b/src/PVE/QemuServer/Drive.pm
> index b80b7dbb..35256acf 100644
> --- a/src/PVE/QemuServer/Drive.pm
> +++ b/src/PVE/QemuServer/Drive.pm
> @@ -23,6 +23,7 @@ our @EXPORT_OK = qw(
>      checked_volume_format
>      drive_is_cloudinit
>      drive_is_cdrom
> +    drive_is_absolute_path

nit: should be sorted alphabetically

>      parse_drive
>      print_drive
>      storage_allows_io_uring_default
> @@ -766,6 +767,11 @@ sub drive_is_cdrom {
>      return $drive && $drive->{media} && ($drive->{media} eq 'cdrom');
>  }
>  
> +sub drive_is_absolute_path {
> +    my ($drive) = @_;
> +    return PVE::QemuServer::Helpers::is_absolute_path($drive->{file});
> +}
> +
>  sub parse_drive_interface {
>      my ($key) = @_;
>  
> @@ -1061,7 +1067,7 @@ sub get_scsi_device_type {
>      if (drive_is_cdrom($drive) || drive_is_cloudinit($drive)) {
>          $devicetype = 'cd';
>      } else {
> -        if ($drive->{file} =~ m|^/|) {
> +        if (drive_is_absolute_path($drive)) {
>              $path = $drive->{file};
>              if (my $info = path_is_scsi($path)) {
>                  if ($info->{type} == 0 && $drive->{scsiblock}) {
> diff --git a/src/PVE/QemuServer/Helpers.pm b/src/PVE/QemuServer/Helpers.pm
> index dd17eef5..5cc3e8b5 100644
> --- a/src/PVE/QemuServer/Helpers.pm
> +++ b/src/PVE/QemuServer/Helpers.pm
> @@ -389,4 +389,9 @@ sub get_host_phys_address_bits {
>      return; # undef, cannot really do anything..
>  }
>  
> +sub is_absolute_path {

the subroutine name is rather short in description, this should be at
least

    volid_is_absolute_path()

or

    volumeid_is_absolute_path()

or something different.

> +    my ($volid) = @_;
> +    return $volid =~ m|^/|;
> +}
> +
>  1;




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH qemu-server v2 2/2] fix #7828: avoid parsing absolute paths as volume ids
  2026-07-22  9:52 ` [PATCH qemu-server v2 2/2] fix #7828: avoid parsing absolute paths as volume ids Thomas Ellmenreich
@ 2026-07-22 13:01   ` Fiona Ebner
  2026-07-22 13:11     ` Thomas Ellmenreich
  2026-07-22 14:10   ` Elias Huhsovitz
  1 sibling, 1 reply; 9+ messages in thread
From: Fiona Ebner @ 2026-07-22 13:01 UTC (permalink / raw)
  To: Thomas Ellmenreich, pve-devel

Am 22.07.26 um 11:53 AM schrieb Thomas Ellmenreich:
> Just like in the case of cdroms, absolute paths to storage drives can be
> skipped when beeing marked as unused as they cannot be owned by the vm.
> 
> Fixes: 5b5c5768 ("display warnings for storage errors or if storage no longer exists")
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  src/PVE/QemuServer.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index ce832c8e..ce0eda74 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -1575,7 +1575,7 @@ sub vmconfig_register_unused_drive {
>          eval { PVE::Storage::vdisk_free($storecfg, $drive->{file}) };
>          warn $@ if $@;
>          delete $conf->{'special-sections'}->{cloudinit};
> -    } elsif (!drive_is_cdrom($drive)) {
> +    } elsif (!drive_is_cdrom($drive) && !drive_is_absolute_path($drive)) {
>          my $volid = $drive->{file};
>          my ($storeid, undef) = PVE::Storage::parse_volume_id($volid);

To fix the regression, I would prefer to use the noerr variant of
parse_volume_id() here, because that is what tells us whether the volume
is PVE-managed or not. We already do that in some other places like
import. And then continue only if we got a result for $storeid.

>          if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {





^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH qemu-server v2 2/2] fix #7828: avoid parsing absolute paths as volume ids
  2026-07-22 13:01   ` Fiona Ebner
@ 2026-07-22 13:11     ` Thomas Ellmenreich
  0 siblings, 0 replies; 9+ messages in thread
From: Thomas Ellmenreich @ 2026-07-22 13:11 UTC (permalink / raw)
  To: Fiona Ebner, pve-devel, d.kral

> To fix the regression, I would prefer to use the noerr variant of
> parse_volume_id() here, because that is what tells us whether the volume
> is PVE-managed or not. We already do that in some other places like
> import. And then continue only if we got a result for $storeid.

That sounds like a better solution for me, what do you think @Daniel?




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH qemu-server v2 1/2] refactor checks for absolute path as drive
  2026-07-22 12:45   ` Daniel Kral
@ 2026-07-22 13:40     ` Thomas Ellmenreich
  2026-07-22 14:01     ` Fiona Ebner
  1 sibling, 0 replies; 9+ messages in thread
From: Thomas Ellmenreich @ 2026-07-22 13:40 UTC (permalink / raw)
  To: Daniel Kral, pve-devel

On Wed Jul 22, 2026 at 2:45 PM CEST, Daniel Kral wrote:

[snip]

> Hm, the !$volid check should be included in the check above as well as
> drive_is_absolute_path() doesn't check this and shouldn't need to.
>
> Though if there's good reason that !$volid will never be true here, this
> can be removed in a previous commit with a clear reasoning why.

Good catch, was a thinking mistake on my part. Thought the `!volid`
check was unnecessary because the regex cleanly handles it, although
that misses the point. Will reintroduce it.

[snip]




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH qemu-server v2 1/2] refactor checks for absolute path as drive
  2026-07-22 12:45   ` Daniel Kral
  2026-07-22 13:40     ` Thomas Ellmenreich
@ 2026-07-22 14:01     ` Fiona Ebner
  1 sibling, 0 replies; 9+ messages in thread
From: Fiona Ebner @ 2026-07-22 14:01 UTC (permalink / raw)
  To: Daniel Kral, Thomas Ellmenreich, pve-devel

Am 22.07.26 um 2:45 PM schrieb Daniel Kral:
> On Wed Jul 22, 2026 at 11:52 AM CEST, Thomas Ellmenreich wrote:
>> Previously there were a bunch of separate checks for whether a drive was
>> an absolute path. These have been refactored to use two checks in
>> QemuServer::Drive and QemuServer::Helpers.

While these helpers are fine at a glance, a classify_drive_volid()
similar to LXC/Config.pm's classify_mountpoint() might be a bit nicer in
the long run. It could return 'absolute', 'none', 'cdrom' and 'volume'.

>> @@ -913,7 +914,7 @@ PVE::JSONSchema::register_format(
>>  sub verify_volume_id_or_absolute_path {
>>      my ($volid, $noerr) = @_;
>>  
>> -    return $volid if $volid =~ m|^/|;
>> +    return $volid if PVE::QemuServer::Helpers::is_absolute_path($volid);
>>  
>>      $volid = eval { PVE::JSONSchema::check_format('pve-volume-id', $volid, '') };
>>      if ($@) {
>> @@ -1555,7 +1556,7 @@ sub print_vga_device {
>>  sub vm_is_volid_owner {
>>      my ($storecfg, $vmid, $volid) = @_;
>>  
>> -    if ($volid !~ m|^/|) {
>> +    if (!PVE::QemuServer::Helpers::is_absolute_path($volid)) {
>>          my ($path, $owner);
>>          eval { ($path, $owner) = PVE::Storage::path($storecfg, $volid); };
>>          log_warn("ownership of volume '$volid' could not be determined: $@") if $@;
>> @@ -1841,11 +1842,9 @@ sub destroy_vm {
>>              { include_unused => 1 },
>>              sub {
>>                  my ($ds, $drive) = @_;
>> -                return if drive_is_cdrom($drive);
>> +                return if drive_is_cdrom($drive) || drive_is_absolute_path($drive);
>>  
>>                  my $volid = $drive->{file};
>> -                return if !$volid || $volid =~ m|^/|;
> 
> Hm, the !$volid check should be included in the check above as well as
> drive_is_absolute_path() doesn't check this and shouldn't need to.
> 
> Though if there's good reason that !$volid will never be true here, this
> can be removed in a previous commit with a clear reasoning why.

We should be protected. We don't allow empty values as part of a
property string and even if, the schema validation against
'pve-volume-id-or-qm-path' would fail. The implementation in
foreach_volume_full() in pve-guest-common won't call the closure if
parsing fails. So I think dropping the check is fine. If it really
triggers, it will just result in a warning pointing to a programming
error somewhere else, which we should fix then.




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH qemu-server v2 2/2] fix #7828: avoid parsing absolute paths as volume ids
  2026-07-22  9:52 ` [PATCH qemu-server v2 2/2] fix #7828: avoid parsing absolute paths as volume ids Thomas Ellmenreich
  2026-07-22 13:01   ` Fiona Ebner
@ 2026-07-22 14:10   ` Elias Huhsovitz
  1 sibling, 0 replies; 9+ messages in thread
From: Elias Huhsovitz @ 2026-07-22 14:10 UTC (permalink / raw)
  To: Thomas Ellmenreich, pve-devel

Small typo, otherwise see review by fiona Ebner [1].

[1:] https://lore.proxmox.com/all/8abb29af-e16d-456b-ab14-cd423abdd801@proxmox.com/

On Wed Jul 22, 2026 at 11:52 AM CEST, Thomas Ellmenreich wrote:
> Just like in the case of cdroms, absolute paths to storage drives can be
> skipped when beeing marked as unused as they cannot be owned by the vm.
		^^^
		small typo
>
> Fixes: 5b5c5768 ("display warnings for storage errors or if storage no longer exists")
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  src/PVE/QemuServer.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index ce832c8e..ce0eda74 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -1575,7 +1575,7 @@ sub vmconfig_register_unused_drive {
>          eval { PVE::Storage::vdisk_free($storecfg, $drive->{file}) };
>          warn $@ if $@;
>          delete $conf->{'special-sections'}->{cloudinit};
> -    } elsif (!drive_is_cdrom($drive)) {
> +    } elsif (!drive_is_cdrom($drive) && !drive_is_absolute_path($drive)) {
>          my $volid = $drive->{file};
>          my ($storeid, undef) = PVE::Storage::parse_volume_id($volid);
>          if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {





^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2026-07-22 14:10 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-22  9:52 [PATCH qemu-server v2 0/2] fix #7828: avoid parsing absolute paths as volume ids Thomas Ellmenreich
2026-07-22  9:52 ` [PATCH qemu-server v2 1/2] refactor checks for absolute path as drive Thomas Ellmenreich
2026-07-22 12:45   ` Daniel Kral
2026-07-22 13:40     ` Thomas Ellmenreich
2026-07-22 14:01     ` Fiona Ebner
2026-07-22  9:52 ` [PATCH qemu-server v2 2/2] fix #7828: avoid parsing absolute paths as volume ids Thomas Ellmenreich
2026-07-22 13:01   ` Fiona Ebner
2026-07-22 13:11     ` Thomas Ellmenreich
2026-07-22 14:10   ` Elias Huhsovitz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal