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

[PATCH qemu-server v2 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' and in the case of
> classification failure: 'unknown'

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 (thanks @Elias)
--------------------------------

- Instead of mixing double quotes (") and single quotes (') the
  'classify_drive_file' function now only uses single quotes.

- Since returning undef for 'classify_drive_file' could lead to
  unnessesary undef checking when comparing to expected values to avoid
  warnings, the subroutine now returns 'unknown' in the case of a failed
  classification.

Changes since RFC (thanks @Max, @Fiona)
---------------------------------------

- 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):
  move verify_volume_id_or_* format registrations into Drive submodule
  add new classify_drive_file utility
  removed some unnecessary undef checks.

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


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

-- 
Generated by murpp 0.12.0

[PATCH qemu-server v2 1/3] move verify_volume_id_or_* format registrations into Drive submodule

[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>
Reviewed-by: Elias Huhsovitz <e.huhsovitz@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

[PATCH qemu-server v2 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 'unknown'.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 src/PVE/QemuServer.pm       | 29 ++++++++++++++++++-----------
 src/PVE/QemuServer/Drive.pm | 25 +++++++++++++++++++++++++
 2 files changed, 43 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..5a9b5ae2 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,24 @@ 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';
+    }
+
+    return 'unknown';
+}
+
 sub drive_is_cloudinit {
     my ($drive) = @_;
     return $drive->{file} =~ m@[:/](?:vm-\d+-)?cloudinit(?:\.$QEMU_FORMAT_RE)?$@;
@@ -794,6 +814,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

[PATCH qemu-server v2 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 v2 2/3] add new classify_drive_file utility

[Max R. Carrara]

On Wed Aug 5, 2026 at 10:21 AM 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 'unknown'.
>
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  src/PVE/QemuServer.pm       | 29 ++++++++++++++++++-----------
>  src/PVE/QemuServer/Drive.pm | 25 +++++++++++++++++++++++++
>  2 files changed, 43 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..5a9b5ae2 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,24 @@ 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';
> +    }
> +
> +    return 'unknown';
> +}

Small note regarding style: While the above is completely fine (and you
shouldn't change it!) I do wanna note that for more complex subroutines,
I would personally prefer avoiding if-elsif chains, mainly because..

1. they tend to become a little too "packed" when it comes to reading the
   code

2. they *may* inadvertently make future changes harder if a little more
   complex logic is involved

To give you an example, have a look at some of our older code in
`pve-storage` [0] that I'm trying to refactor at the moment.

Eventually I replace each branch one-by-one in my series [1], until I
finally condense the refactored logic in (yet) another patch [2].

Again, the code above is completely fine! Please don't feel like you
have to refresh your series. I just wanted to mention it for future
cases where the chains might be longer and the conditions are nastier :P

[0]: https://git.proxmox.com/?p=pve-storage.git;a=blob;f=src/PVE/Storage/Plugin.pm;h=4f69f9b5db69674335eb3024d61d4a3430bca1ec;hb=refs/heads/master#l799
[1]: https://lore.proxmox.com/pve-devel/20260422111322.257380-23-m.carrara@proxmox.com/#Z31src:PVE:Storage:Plugin.pm
[2]: https://lore.proxmox.com/pve-devel/20260422111322.257380-30-m.carrara@proxmox.com/#Z31src:PVE:Storage:Plugin.pm

> +
>  sub drive_is_cloudinit {
>      my ($drive) = @_;
>      return $drive->{file} =~ m@[:/](?:vm-\d+-)?cloudinit(?:\.$QEMU_FORMAT_RE)?$@;
> @@ -794,6 +814,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) = @_;
>

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

[Max R. Carrara]

^ Small note regarding commit summaries and messages: We use present
tense with active voice / imperative mood and don't add any fullstops in
summaries.

So, the summary line should instead say:

  remove some unnecessary undef checks

IMO this can be changed when applying the patch, no need for a refresh
here either.

On Wed Aug 5, 2026 at 10:21 AM 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);
> +                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);
>

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

[Thomas Ellmenreich]

On Fri Aug 7, 2026 at 11:30 AM CEST, Max R. Carrara wrote:

[snip]

>> +# 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';
>> +    }
>> +
>> +    return 'unknown';
>> +}
>
> Small note regarding style: While the above is completely fine (and you
> shouldn't change it!) I do wanna note that for more complex subroutines,
> I would personally prefer avoiding if-elsif chains, mainly because..
>
> 1. they tend to become a little too "packed" when it comes to reading the
>    code
>
> 2. they *may* inadvertently make future changes harder if a little more
>    complex logic is involved
>
> To give you an example, have a look at some of our older code in
> `pve-storage` [0] that I'm trying to refactor at the moment.
>
> Eventually I replace each branch one-by-one in my series [1], until I
> finally condense the refactored logic in (yet) another patch [2].
>
> Again, the code above is completely fine! Please don't feel like you
> have to refresh your series. I just wanted to mention it for future
> cases where the chains might be longer and the conditions are nastier :P
>
> [0]: https://git.proxmox.com/?p=pve-storage.git;a=blob;f=src/PVE/Storage/Plugin.pm;h=4f69f9b5db69674335eb3024d61d4a3430bca1ec;hb=refs/heads/master#l799
> [1]: https://lore.proxmox.com/pve-devel/20260422111322.257380-23-m.carrara@proxmox.com/#Z31src:PVE:Storage:Plugin.pm
> [2]: https://lore.proxmox.com/pve-devel/20260422111322.257380-30-m.carrara@proxmox.com/#Z31src:PVE:Storage:Plugin.pm

I totally agree, I also found the style a bit too dense for my liking, but
was not sure what the better option would be. In the future, I will try to
avoid it and go for a less "chainy" method.

Also regarding the other mail about the imperative subject, it's also a
thing I have to properly get used to. (the fullstop was a typo)

Thanks for pointing these things out. ;)

[snip]

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

[Elias Huhsovitz]

Good refactor.

1 small nit inline.

On Wed Aug 5, 2026 at 10:21 AM 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 'unknown'.
>
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  src/PVE/QemuServer.pm       | 29 ++++++++++++++++++-----------
>  src/PVE/QemuServer/Drive.pm | 25 +++++++++++++++++++++++++
>  2 files changed, 43 insertions(+), 11 deletions(-)
>
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index fdef20dc..0e998cbe 100644

[...]

> +# Tries to classify the drive file as either 'none', 'cdrom', 'absolute'
> +# or 'volume'. In all other cases it will return undef.

nit: Subroutine returns 'unknown' in default path.
Comments states that `undef` is returned.

> +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';
> +    }
> +
> +    return 'unknown';
> +}
> +
>  sub drive_is_cloudinit {
>      my ($drive) = @_;
>      return $drive->{file} =~ m@[:/](?:vm-\d+-)?cloudinit(?:\.$QEMU_FORMAT_RE)?$@;
> @@ -794,6 +814,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) = @_;
>