[PATCH qemu-server 0/3] refactor of volume_id classification

[PATCH qemu-server 0/3] refactor of volume_id classification

[Thomas Ellmenreich]

Refactor of volume_id classification
====================================

While developing this: [0] patch series, I refactored all the checks for
disk files as absolute paths into their own subroutine. The original series
was then dropped in favour of a quicker fix: [1]. This series revives the
refactor without the fix and, unlike the original series creates a new
classify_drive_file subroutine, as @Fiona Ebener mentioned here: [2].

Overview
--------

As previously mentioned, a new classify_drive_file subroutine has been
introduced, which can then be used to determine the type of disk file. This
type can be one of:

> 'none', 'cdrom', 'absolute', 'volume'

Other Small changes
-------------------

In the light of this small refactoring, 2 other small changes crept in.
(1) First, one to move the definitions and registration of 2 drive related
subroutines to the QemuServer::Drive submodule. Then (2), the removal of a
couple of undef checks for volids which are no longer necessary. These
changes have been placed in their own patches to be easily reverted.

Changes since v1
----------------

- moved the classification subroutine out of PVE::Storage and into
  PVE::QemuServer::Drive

- Rewrite of the subroutine with the following changes:
  - does not throw errors anymore, only returns undef if
    classification fails
  - renamed to 'classify_drive_file'
  - before, undef led to it being classified as 'none' which now
    also just returns undef

- moved the registration of 'verify_volume_id_or_(absolute_path/qm_path)'
  from QemuServer to QemuServer::Drive

- reintroduced the removal of some unnecessary undef checks as a
  separate patch

[0]: https://lore.proxmox.com/pve-devel/20260722095251.89606-1-t.ellmenreich@proxmox.com/T/#t
[1]: https://lore.proxmox.com/pve-devel/20260724081611.11254-1-f.ebner@proxmox.com/
[2]: https://lore.proxmox.com/pve-devel/0b4272e1-3634-4fdd-b79f-6b077dd387d5@proxmox.com/


qemu-manager:

Thomas Ellmenreich (3):
  moved verify_volume_id_or_* format registrations
  add new classify_drive_file utility
  removed some unnecessary undef checks.

 src/PVE/QemuServer.pm       | 63 +++++++++++--------------------------
 src/PVE/QemuServer/Drive.pm | 51 ++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 44 deletions(-)


Summary over all repositories:
  2 files changed, 70 insertions(+), 44 deletions(-)

-- 
Generated by murpp 0.12.0

[PATCH qemu-server 1/3] moved verify_volume_id_or_* format registrations

[Thomas Ellmenreich]

The format registrations of verify_volume_id_or_qm_path and
verify_volume_id_or_absolute_path were moved from QemuServer to
QemuServer::Drive, which is a more fitting location.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 src/PVE/QemuServer.pm       | 28 ----------------------------
 src/PVE/QemuServer/Drive.pm | 28 ++++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 9aec7f9c..fdef20dc 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -895,34 +895,6 @@ sub pve_verify_cpuset {
     return PVE::CpuSet->new($members)->short_string();
 }
 
-PVE::JSONSchema::register_format('pve-volume-id-or-qm-path', \&verify_volume_id_or_qm_path);
-
-sub verify_volume_id_or_qm_path {
-    my ($volid, $noerr) = @_;
-
-    return $volid if $volid eq 'none' || $volid eq 'cdrom';
-
-    return verify_volume_id_or_absolute_path($volid, $noerr);
-}
-
-PVE::JSONSchema::register_format(
-    'pve-volume-id-or-absolute-path',
-    \&verify_volume_id_or_absolute_path,
-);
-
-sub verify_volume_id_or_absolute_path {
-    my ($volid, $noerr) = @_;
-
-    return $volid if $volid =~ m|^/|;
-
-    $volid = eval { PVE::JSONSchema::check_format('pve-volume-id', $volid, '') };
-    if ($@) {
-        return if $noerr;
-        die $@;
-    }
-    return $volid;
-}
-
 my $serialdesc = {
     optional => 1,
     type => 'string',
diff --git a/src/PVE/QemuServer/Drive.pm b/src/PVE/QemuServer/Drive.pm
index b80b7dbb..52e4a89b 100644
--- a/src/PVE/QemuServer/Drive.pm
+++ b/src/PVE/QemuServer/Drive.pm
@@ -753,6 +753,34 @@ sub verify_bootdisk {
     die "invalid boot disk '$value'\n";
 }
 
+PVE::JSONSchema::register_format('pve-volume-id-or-qm-path', \&verify_volume_id_or_qm_path);
+
+sub verify_volume_id_or_qm_path {
+    my ($volid, $noerr) = @_;
+
+    return $volid if $volid eq 'none' || $volid eq 'cdrom';
+
+    return verify_volume_id_or_absolute_path($volid, $noerr);
+}
+
+PVE::JSONSchema::register_format(
+    'pve-volume-id-or-absolute-path',
+    \&verify_volume_id_or_absolute_path,
+);
+
+sub verify_volume_id_or_absolute_path {
+    my ($volid, $noerr) = @_;
+
+    return $volid if $volid =~ m|^/|;
+
+    $volid = eval { PVE::JSONSchema::check_format('pve-volume-id', $volid, '') };
+    if ($@) {
+        return if $noerr;
+        die $@;
+    }
+    return $volid;
+}
+
 sub drive_is_cloudinit {
     my ($drive) = @_;
     return $drive->{file} =~ m@[:/](?:vm-\d+-)?cloudinit(?:\.$QEMU_FORMAT_RE)?$@;
-- 
2.47.3

Re: [PATCH qemu-server 1/3] moved verify_volume_id_or_* format registrations

[Elias Huhsovitz]

Good change. The QemuServer.pm is quite long and this refactor makes
sense. One small nit inline, otherwise consider this:

Reviewed-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>

On Mon Aug 3, 2026 at 2:12 PM CEST, Thomas Ellmenreich wrote:
> The format registrations of verify_volume_id_or_qm_path and
> verify_volume_id_or_absolute_path were moved from QemuServer to
> QemuServer::Drive, which is a more fitting location.

nit: Commit messages should be written in imperative.

e.g., "move verify_volume_id_or_* format registrations from X to Y"
instead of "verify_volume_id_or_* were moved from X to Y"

> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  src/PVE/QemuServer.pm       | 28 ----------------------------
>  src/PVE/QemuServer/Drive.pm | 28 ++++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+), 28 deletions(-)
>
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 9aec7f9c..fdef20dc 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -895,34 +895,6 @@ sub pve_verify_cpuset {
>      return PVE::CpuSet->new($members)->short_string();
>  }
>  
> -PVE::JSONSchema::register_format('pve-volume-id-or-qm-path', \&verify_volume_id_or_qm_path);
> -
> -sub verify_volume_id_or_qm_path {
> -    my ($volid, $noerr) = @_;
> -
> -    return $volid if $volid eq 'none' || $volid eq 'cdrom';
> -
> -    return verify_volume_id_or_absolute_path($volid, $noerr);
> -}
> -
> -PVE::JSONSchema::register_format(
> -    'pve-volume-id-or-absolute-path',
> -    \&verify_volume_id_or_absolute_path,
> -);
> -
> -sub verify_volume_id_or_absolute_path {
> -    my ($volid, $noerr) = @_;
> -
> -    return $volid if $volid =~ m|^/|;
> -
> -    $volid = eval { PVE::JSONSchema::check_format('pve-volume-id', $volid, '') };
> -    if ($@) {
> -        return if $noerr;
> -        die $@;
> -    }
> -    return $volid;
> -}
> -
>  my $serialdesc = {
>      optional => 1,
>      type => 'string',
> diff --git a/src/PVE/QemuServer/Drive.pm b/src/PVE/QemuServer/Drive.pm
> index b80b7dbb..52e4a89b 100644
> --- a/src/PVE/QemuServer/Drive.pm
> +++ b/src/PVE/QemuServer/Drive.pm
> @@ -753,6 +753,34 @@ sub verify_bootdisk {
>      die "invalid boot disk '$value'\n";
>  }
>  
> +PVE::JSONSchema::register_format('pve-volume-id-or-qm-path', \&verify_volume_id_or_qm_path);
> +
> +sub verify_volume_id_or_qm_path {
> +    my ($volid, $noerr) = @_;
> +
> +    return $volid if $volid eq 'none' || $volid eq 'cdrom';
> +
> +    return verify_volume_id_or_absolute_path($volid, $noerr);
> +}
> +
> +PVE::JSONSchema::register_format(
> +    'pve-volume-id-or-absolute-path',
> +    \&verify_volume_id_or_absolute_path,
> +);
> +
> +sub verify_volume_id_or_absolute_path {
> +    my ($volid, $noerr) = @_;
> +
> +    return $volid if $volid =~ m|^/|;
> +
> +    $volid = eval { PVE::JSONSchema::check_format('pve-volume-id', $volid, '') };
> +    if ($@) {
> +        return if $noerr;
> +        die $@;
> +    }
> +    return $volid;
> +}
> +
>  sub drive_is_cloudinit {
>      my ($drive) = @_;
>      return $drive->{file} =~ m@[:/](?:vm-\d+-)?cloudinit(?:\.$QEMU_FORMAT_RE)?$@;

[PATCH qemu-server 2/3] add new classify_drive_file utility

[Thomas Ellmenreich]

To more easily and clearly branch off different types of drive files a
new utility function is introduced that classifies such a drive file.
The return options are 'none', 'cdrom', 'absolute' and 'volume'. In any
other case the function returns undef.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 src/PVE/QemuServer.pm       | 29 ++++++++++++++++++-----------
 src/PVE/QemuServer/Drive.pm | 23 +++++++++++++++++++++++
 2 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index fdef20dc..0e998cbe 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -43,7 +43,6 @@ use PVE::PBSClient;
 use PVE::RESTEnvironment qw(log_warn);
 use PVE::RPCEnvironment;
 use PVE::SafeSyslog;
-use PVE::Storage;
 use PVE::SysFSTools;
 use PVE::Systemd;
 use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_foreach $IPV6RE);
@@ -73,6 +72,8 @@ use PVE::QemuServer::CPUFlags;
 use PVE::QemuServer::Drive qw(
     is_valid_drivename
     checked_volume_format
+    classify_drive_file
+    drive_has_absolute_path
     drive_is_cloudinit
     drive_is_cdrom
     parse_drive
@@ -1102,7 +1103,10 @@ sub cleanup_drive_path {
         $drive->{file} = $volid;
     }
 
-    $drive->{media} = 'cdrom' if !$drive->{media} && $drive->{file} =~ m/^(cdrom|none)$/;
+    my $file_type = classify_drive_file($drive->{file});
+
+    $drive->{media} = 'cdrom'
+        if !$drive->{media} && ($file_type eq "cdrom" || $file_type eq "none");
 }
 
 sub parse_hotplug_features {
@@ -1527,7 +1531,7 @@ sub print_vga_device {
 sub vm_is_volid_owner {
     my ($storecfg, $vmid, $volid) = @_;
 
-    if ($volid !~ m|^/|) {
+    if (classify_drive_file($volid) ne "absolute") {
         my ($path, $owner);
         eval { ($path, $owner) = PVE::Storage::path($storecfg, $volid); };
         log_warn("ownership of volume '$volid' could not be determined: $@") if $@;
@@ -1817,7 +1821,7 @@ sub destroy_vm {
                 return if drive_is_cdrom($drive);
 
                 my $volid = $drive->{file};
-                return if !$volid || $volid =~ m|^/|;
+                return if !$volid || drive_has_absolute_path($drive);
 
                 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
@@ -1835,7 +1839,7 @@ sub destroy_vm {
         return if drive_is_cdrom($drive, 1);
 
         my $volid = $drive->{file};
-        return if !$volid || $volid =~ m|^/|;
+        return if !$volid || drive_has_absolute_path($drive);
         return if $volids->{$volid};
 
         my ($path, $owner) = eval { PVE::Storage::path($storecfg, $volid) };
@@ -3597,7 +3601,7 @@ sub config_to_command {
             my $live_restore = $live_restore_backing->{$ds};
 
             if (min_version($machine_version, 10, 0)) { # for the switch to -blockdev
-                if ($drive->{file} ne 'none') {
+                if (classify_drive_file($drive->{file}) ne "none") {
                     my $throttle_group =
                         PVE::QemuServer::Blockdev::generate_throttle_group($drive);
                     push @$cmd, '-object', to_json($throttle_group, { canonical => 1 });
@@ -5276,7 +5280,10 @@ sub vmconfig_update_disk {
             eval { PVE::QemuServer::Blockdev::change_medium($storecfg, $vmid, $opt, $drive); };
             my $err = $@;
 
-            if ($drive->{file} eq 'none' && drive_is_cloudinit($old_drive)) {
+            if (
+                classify_drive_file($drive->{file}) eq "none"
+                && drive_is_cloudinit($old_drive)
+            ) {
                 vmconfig_register_unused_drive($storecfg, $vmid, $conf, $old_drive);
             }
 
@@ -6058,7 +6065,7 @@ sub get_vm_volumes {
         sub {
             my ($volid, $attr) = @_;
 
-            return if $volid =~ m|^/|;
+            return if classify_drive_file($volid) eq "absolute";
 
             my ($sid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
             return if !$sid;
@@ -6082,7 +6089,7 @@ sub get_current_vm_volumes {
         sub {
             my ($ds, $drive) = @_;
 
-            if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
+            if (classify_drive_file($drive->{file}) eq "volume") {
                 check_volume_storage_type($storecfg, $drive->{file});
                 push $volumes->@*, $drive->{file};
             }
@@ -6458,7 +6465,7 @@ sub tar_restore_cleanup {
             if ($line =~ m/vzdump:([^\s:]*):(\S+)$/) {
                 my $volid = $2;
                 eval {
-                    if ($volid =~ m|^/|) {
+                    if (classify_drive_file($volid) eq "absolute") {
                         unlink $volid || die 'unlink failed\n';
                     } else {
                         PVE::Storage::vdisk_free($storecfg, $volid);
@@ -6506,7 +6513,7 @@ my $restore_cleanup_oldconf = sub {
             return if drive_is_cdrom($drive, 1);
 
             my $volid = $drive->{file};
-            return if !$volid || $volid =~ m|^/|;
+            return if !$volid || drive_has_absolute_path($drive);
 
             my ($path, $owner) = PVE::Storage::path($storecfg, $volid);
             return if !$path || !$owner || ($owner != $vmid);
diff --git a/src/PVE/QemuServer/Drive.pm b/src/PVE/QemuServer/Drive.pm
index 52e4a89b..f2106141 100644
--- a/src/PVE/QemuServer/Drive.pm
+++ b/src/PVE/QemuServer/Drive.pm
@@ -21,6 +21,8 @@ our @EXPORT_OK = qw(
     is_valid_drivename
     checked_parse_volname
     checked_volume_format
+    classify_drive_file
+    drive_has_absolute_path
     drive_is_cloudinit
     drive_is_cdrom
     parse_drive
@@ -781,6 +783,22 @@ sub verify_volume_id_or_absolute_path {
     return $volid;
 }
 
+# Tries to classify the drive file as either 'none', 'cdrom', 'absolute'
+# or 'volume'. In all other cases it will return undef.
+sub classify_drive_file {
+    my ($volid) = @_;
+
+    if ($volid eq "none") {
+        return "none";
+    } elsif ($volid eq 'cdrom') {
+        return "cdrom";
+    } elsif ($volid =~ m|^/|) {
+        return "absolute";
+    } elsif (PVE::Storage::parse_volume_id($volid, 1)) {
+        return "volume";
+    }
+}
+
 sub drive_is_cloudinit {
     my ($drive) = @_;
     return $drive->{file} =~ m@[:/](?:vm-\d+-)?cloudinit(?:\.$QEMU_FORMAT_RE)?$@;
@@ -794,6 +812,11 @@ sub drive_is_cdrom {
     return $drive && $drive->{media} && ($drive->{media} eq 'cdrom');
 }
 
+sub drive_has_absolute_path {
+    my ($drive) = @_;
+    return classify_drive_file($drive->{file}) eq "absolute";
+}
+
 sub parse_drive_interface {
     my ($key) = @_;
 
-- 
2.47.3

Re: [PATCH qemu-server 2/3] add new classify_drive_file utility

[Elias Huhsovitz]

Good idea to unify the checking for drive type!

Summary
-------
* no explicit default return path
* undef return depends on perl version & previous code.
* inconsistent string quotes
* small bug near your changes

Comments inline

On Mon Aug 3, 2026 at 2:12 PM CEST, Thomas Ellmenreich wrote:
> To more easily and clearly branch off different types of drive files a
> new utility function is introduced that classifies such a drive file.
> The return options are 'none', 'cdrom', 'absolute' and 'volume'. In any
> other case the function returns undef.
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  src/PVE/QemuServer.pm       | 29 ++++++++++++++++++-----------
>  src/PVE/QemuServer/Drive.pm | 23 +++++++++++++++++++++++
>  2 files changed, 41 insertions(+), 11 deletions(-)
>
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index fdef20dc..0e998cbe 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -43,7 +43,6 @@ use PVE::PBSClient;
>  use PVE::RESTEnvironment qw(log_warn);
>  use PVE::RPCEnvironment;
>  use PVE::SafeSyslog;
> -use PVE::Storage;
>  use PVE::SysFSTools;
>  use PVE::Systemd;
>  use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_foreach $IPV6RE);
> @@ -73,6 +72,8 @@ use PVE::QemuServer::CPUFlags;
>  use PVE::QemuServer::Drive qw(
>      is_valid_drivename
>      checked_volume_format
> +    classify_drive_file
> +    drive_has_absolute_path
>      drive_is_cloudinit
>      drive_is_cdrom
>      parse_drive
> @@ -1102,7 +1103,10 @@ sub cleanup_drive_path {
>          $drive->{file} = $volid;
>      }
>  
> -    $drive->{media} = 'cdrom' if !$drive->{media} && $drive->{file} =~ m/^(cdrom|none)$/;
> +    my $file_type = classify_drive_file($drive->{file});

	^^^^^
	This variable is not ready to be undef. (See comments on classify_drive_file below)

> +
> +    $drive->{media} = 'cdrom'
> +        if !$drive->{media} && ($file_type eq "cdrom" || $file_type eq "none");
>  }
>  
>  sub parse_hotplug_features {
> @@ -1527,7 +1531,7 @@ sub print_vga_device {
>  sub vm_is_volid_owner {
>      my ($storecfg, $vmid, $volid) = @_;
>  
> -    if ($volid !~ m|^/|) {
> +    if (classify_drive_file($volid) ne "absolute") {
>          my ($path, $owner);
>          eval { ($path, $owner) = PVE::Storage::path($storecfg, $volid); };
>          log_warn("ownership of volume '$volid' could not be determined: $@") if $@;
> @@ -1817,7 +1821,7 @@ sub destroy_vm {
>                  return if drive_is_cdrom($drive);
>  
>                  my $volid = $drive->{file};
> -                return if !$volid || $volid =~ m|^/|;
> +                return if !$volid || drive_has_absolute_path($drive);
>  
>                  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
> @@ -1835,7 +1839,7 @@ sub destroy_vm {
>          return if drive_is_cdrom($drive, 1);
>  
>          my $volid = $drive->{file};
> -        return if !$volid || $volid =~ m|^/|;
> +        return if !$volid || drive_has_absolute_path($drive);
>          return if $volids->{$volid};
>  
>          my ($path, $owner) = eval { PVE::Storage::path($storecfg, $volid) };
> @@ -3597,7 +3601,7 @@ sub config_to_command {
>              my $live_restore = $live_restore_backing->{$ds};
>  
>              if (min_version($machine_version, 10, 0)) { # for the switch to -blockdev
> -                if ($drive->{file} ne 'none') {
> +                if (classify_drive_file($drive->{file}) ne "none") {
>                      my $throttle_group =
>                          PVE::QemuServer::Blockdev::generate_throttle_group($drive);
>                      push @$cmd, '-object', to_json($throttle_group, { canonical => 1 });
> @@ -5276,7 +5280,10 @@ sub vmconfig_update_disk {
>              eval { PVE::QemuServer::Blockdev::change_medium($storecfg, $vmid, $opt, $drive); };
>              my $err = $@;
>  
> -            if ($drive->{file} eq 'none' && drive_is_cloudinit($old_drive)) {
> +            if (
> +                classify_drive_file($drive->{file}) eq "none"
> +                && drive_is_cloudinit($old_drive)
> +            ) {
>                  vmconfig_register_unused_drive($storecfg, $vmid, $conf, $old_drive);
>              }
>  
> @@ -6058,7 +6065,7 @@ sub get_vm_volumes {
>          sub {
>              my ($volid, $attr) = @_;
>  
> -            return if $volid =~ m|^/|;
> +            return if classify_drive_file($volid) eq "absolute";
>  
>              my ($sid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
>              return if !$sid;
> @@ -6082,7 +6089,7 @@ sub get_current_vm_volumes {
>          sub {
>              my ($ds, $drive) = @_;
>  
> -            if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
> +            if (classify_drive_file($drive->{file}) eq "volume") {
>                  check_volume_storage_type($storecfg, $drive->{file});
>                  push $volumes->@*, $drive->{file};
>              }
> @@ -6458,7 +6465,7 @@ sub tar_restore_cleanup {
>              if ($line =~ m/vzdump:([^\s:]*):(\S+)$/) {
>                  my $volid = $2;
>                  eval {
> -                    if ($volid =~ m|^/|) {
> +                    if (classify_drive_file($volid) eq "absolute") {
>                          unlink $volid || die 'unlink failed\n';

Should be "unlink failed\n" instead of 'unlink failed'. Otherwise we
render the literal '\' + 'n'.

I know this isn't your change, but i noticed this ;D.

>                      } else {
>                          PVE::Storage::vdisk_free($storecfg, $volid);
> @@ -6506,7 +6513,7 @@ my $restore_cleanup_oldconf = sub {
>              return if drive_is_cdrom($drive, 1);
>  
>              my $volid = $drive->{file};
> -            return if !$volid || $volid =~ m|^/|;
> +            return if !$volid || drive_has_absolute_path($drive);
>  
>              my ($path, $owner) = PVE::Storage::path($storecfg, $volid);
>              return if !$path || !$owner || ($owner != $vmid);
> diff --git a/src/PVE/QemuServer/Drive.pm b/src/PVE/QemuServer/Drive.pm
> index 52e4a89b..f2106141 100644
> --- a/src/PVE/QemuServer/Drive.pm
> +++ b/src/PVE/QemuServer/Drive.pm
> @@ -21,6 +21,8 @@ our @EXPORT_OK = qw(
>      is_valid_drivename
>      checked_parse_volname
>      checked_volume_format
> +    classify_drive_file
> +    drive_has_absolute_path
>      drive_is_cloudinit
>      drive_is_cdrom
>      parse_drive
> @@ -781,6 +783,22 @@ sub verify_volume_id_or_absolute_path {
>      return $volid;
>  }
>  
> +# Tries to classify the drive file as either 'none', 'cdrom', 'absolute'
> +# or 'volume'. In all other cases it will return undef.
> +sub classify_drive_file {
> +    my ($volid) = @_;
> +
> +    if ($volid eq "none") {
> +        return "none";
> +    } elsif ($volid eq 'cdrom') {

Different string quotes for 'cdrom'. Other strings have double quotes.
e.g., "none", "cdrom".

I would suggest to only use single quotes here, since we do not have any
string interpolation anyways.

> +        return "cdrom";
> +    } elsif ($volid =~ m|^/|) {
> +        return "absolute";
> +    } elsif (PVE::Storage::parse_volume_id($volid, 1)) {
> +        return "volume";
> +    }
> +}

Missing default return path. The comments claim that the default path is
undef, but this is not necessarily true and depends on the perl version
and or the last executed statement.

Even if the default path is always undef, this might lead to issues down
the line. Some callers of this function are not prepared to handle undef
(see comment futher up in the `cleanup_drive_path` subroutine). I would suggest returning a string that
singals an error, e.g., an empty string ''

> +
>  sub drive_is_cloudinit {
>      my ($drive) = @_;
>      return $drive->{file} =~ m@[:/](?:vm-\d+-)?cloudinit(?:\.$QEMU_FORMAT_RE)?$@;
> @@ -794,6 +812,11 @@ sub drive_is_cdrom {
>      return $drive && $drive->{media} && ($drive->{media} eq 'cdrom');
>  }
>  
> +sub drive_has_absolute_path {
> +    my ($drive) = @_;
> +    return classify_drive_file($drive->{file}) eq "absolute";
> +}
> +
>  sub parse_drive_interface {
>      my ($key) = @_;
>  

[PATCH qemu-server 3/3] removed some unnecessary undef checks.

[Thomas Ellmenreich]

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 src/PVE/QemuServer.pm | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 0e998cbe..3141b298 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -1818,11 +1818,10 @@ sub destroy_vm {
             { include_unused => 1 },
             sub {
                 my ($ds, $drive) = @_;
-                return if drive_is_cdrom($drive);
 
-                my $volid = $drive->{file};
-                return if !$volid || drive_has_absolute_path($drive);
+                return if drive_is_cdrom($drive) || drive_has_absolute_path($drive);
 
+                my $volid = $drive->{file};
                 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: $@")
@@ -1839,8 +1838,7 @@ sub destroy_vm {
         return if drive_is_cdrom($drive, 1);
 
         my $volid = $drive->{file};
-        return if !$volid || drive_has_absolute_path($drive);
-        return if $volids->{$volid};
+        return if drive_has_absolute_path($drive) || $volids->{$volid};
 
         my ($path, $owner) = eval { PVE::Storage::path($storecfg, $volid) };
         log_warn("failed to get path and owner of volume '$volid': $@") if $@;
@@ -6510,11 +6508,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_has_absolute_path($drive);
 
             my $volid = $drive->{file};
-            return if !$volid || drive_has_absolute_path($drive);
-
             my ($path, $owner) = PVE::Storage::path($storecfg, $volid);
             return if !$path || !$owner || ($owner != $vmid);
 
-- 
2.47.3

Re: [PATCH qemu-server 3/3] removed some unnecessary undef checks.

[Elias Huhsovitz]

Summary
-------
* classify_drive_file() and drive_has_absolute_path() are not prepared
  to handle undef values, and therefore removing the undef checks causes
  a change in behaviour

* commit message should be imperative, e.g., "remove some unnecessary
undef checks"

On Mon Aug 3, 2026 at 2:12 PM CEST, Thomas Ellmenreich wrote:
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  src/PVE/QemuServer.pm | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 0e998cbe..3141b298 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -1818,11 +1818,10 @@ sub destroy_vm {
>              { include_unused => 1 },
>              sub {
>                  my ($ds, $drive) = @_;
> -                return if drive_is_cdrom($drive);
>  
> -                my $volid = $drive->{file};
> -                return if !$volid || drive_has_absolute_path($drive);

I think this might result in an edge case where $drive->{file} can be undef,
causing the underlying function classify_drive_file() that drive_has_absolute_path()
calls, to emit a warning.

To my understanding, classify_drive_file() is currently unpreparted to
receive undef values. I believe a guard clause in classify_drive_file()
resolves this.

e.g., in classify_drive_file:

my ($volid) = @_;
return '' if (!$volid);

> +                return if drive_is_cdrom($drive) || drive_has_absolute_path($drive);
>  
> +                my $volid = $drive->{file};
>                  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: $@")
> @@ -1839,8 +1838,7 @@ sub destroy_vm {
>          return if drive_is_cdrom($drive, 1);
>  
>          my $volid = $drive->{file};
> -        return if !$volid || drive_has_absolute_path($drive);

same here.

> -        return if $volids->{$volid};
> +        return if drive_has_absolute_path($drive) || $volids->{$volid};
>  
>          my ($path, $owner) = eval { PVE::Storage::path($storecfg, $volid) };
>          log_warn("failed to get path and owner of volume '$volid': $@") if $@;
> @@ -6510,11 +6508,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_has_absolute_path($drive);
>  
>              my $volid = $drive->{file};
> -            return if !$volid || drive_has_absolute_path($drive);

same here.

> -
>              my ($path, $owner) = PVE::Storage::path($storecfg, $volid);
>              return if !$path || !$owner || ($owner != $vmid);
>  

Re: [PATCH qemu-server 3/3] removed some unnecessary undef checks.

[Fiona Ebner]

Am 05.08.26 um 10:24 AM schrieb Elias Huhsovitz:
> Summary
> -------
> * classify_drive_file() and drive_has_absolute_path() are not prepared
>   to handle undef values, and therefore removing the undef checks causes
>   a change in behaviour
> 
> * commit message should be imperative, e.g., "remove some unnecessary
> undef checks"
> 
> On Mon Aug 3, 2026 at 2:12 PM CEST, Thomas Ellmenreich wrote:
>> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
>> ---
>>  src/PVE/QemuServer.pm | 12 ++++--------
>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
>> index 0e998cbe..3141b298 100644
>> --- a/src/PVE/QemuServer.pm
>> +++ b/src/PVE/QemuServer.pm
>> @@ -1818,11 +1818,10 @@ sub destroy_vm {
>>              { include_unused => 1 },
>>              sub {
>>                  my ($ds, $drive) = @_;
>> -                return if drive_is_cdrom($drive);
>>  
>> -                my $volid = $drive->{file};
>> -                return if !$volid || drive_has_absolute_path($drive);
> 
> I think this might result in an edge case where $drive->{file} can be undef,
> causing the underlying function classify_drive_file() that drive_has_absolute_path()
> calls, to emit a warning.

No, because the foreach_volume_full() function parses the drive string
and skips without calling the subroutine if parsing fails. If parsing
succeeds, $drive->{file} is populated. Of course the rationale for why
removing the checks is justified should be stated in the commit message.

superseded: [PATCH qemu-server 0/3] refactor of volume_id classification

[Thomas Ellmenreich]

Superseded-by:
https://lore.proxmox.com/pve-devel/20260805082122.43184-1-t.ellmenreich@proxmox.com/T/#t