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

["Max R. Carrara" <m.carrara@proxmox.com>]

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) = @_;
>