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

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

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