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

["Elias Huhsovitz" <e.huhsovitz@proxmox.com>]

Testing
-------
I tested this using qm create with all affected drive types, e.g.,
qm create <vmid> 
--ide2 none,media=crom | cdrom | etc 
--scsi0 local-lvm:5 | etc

and then destroying them using
qm destroy <vmid> --purge

It worked as intended.

Small note
----------
Small note inline, but no change necessary IMHO.
Therefore:

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

On Wed Aug 12, 2026 at 2:13 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 'unknown'.
>
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  src/PVE/QemuServer.pm       | 29 ++++++++++++++++++-----------
>  src/PVE/QemuServer/Drive.pm | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+), 11 deletions(-)
>
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 6c597e85..d9d76ab3 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 {
> @@ -1540,7 +1544,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 $@;
> @@ -1830,7 +1834,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
> @@ -1848,7 +1852,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) };
> @@ -3610,7 +3614,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 });
> @@ -5289,7 +5293,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);
>              }
>  
> @@ -6071,7 +6078,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;
> @@ -6095,7 +6102,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};
>              }
> @@ -6471,7 +6478,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);
> @@ -6519,7 +6526,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..efcfa2b1 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,30 @@ 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 'unknown'.
> +sub classify_drive_file {
> +    my ($volid) = @_;
> +
> +    return 'unknown' if !$volid;
> +
> +    if ($volid eq 'none') {
> +        return 'none';
> +    }
> +
> +    if ($volid eq 'cdrom') {
> +        return 'cdrom';
> +    }
> +
> +    if ($volid =~ m|^/|) {
> +        return 'absolute';
> +    }
> +
> +    return PVE::Storage::parse_volume_id($volid, 1)
> +        ? 'volume'
> +        : 'unknown';
> +}

I think the previous if-else based approach was fine here, but this is
also good. I just noticed one thing regarding this approach. Since we
are no longer mimicking a switch-case statement we could combine the the
'none' and 'cdrom' cases into one like this:

if ($volid eq 'none' || $volid eq 'cdrom') {
        return $volid;
}

But i think the current approach is good and you don't need to change this!

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