Re: [PATCH qemu-server v2 1/2] refactor checks for absolute path as drive

["Daniel Kral" <d.kral@proxmox.com>]

Thanks for the quick v2!

On Wed Jul 22, 2026 at 11:52 AM CEST, Thomas Ellmenreich wrote:
> Previously there were a bunch of separate checks for whether a drive was
> an absolute path. These have been refactored to use two checks in
> QemuServer::Drive and QemuServer::Helpers.
>
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  src/PVE/QemuConfig.pm          |  2 +-
>  src/PVE/QemuMigrate.pm         |  2 +-
>  src/PVE/QemuServer.pm          | 20 ++++++++------------
>  src/PVE/QemuServer/Blockdev.pm |  4 ++--
>  src/PVE/QemuServer/Drive.pm    |  8 +++++++-
>  src/PVE/QemuServer/Helpers.pm  |  5 +++++
>  6 files changed, 24 insertions(+), 17 deletions(-)
>

[ snip ]

> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 191ae549..ce832c8e 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -75,6 +75,7 @@ use PVE::QemuServer::Drive qw(
>      checked_volume_format
>      drive_is_cloudinit
>      drive_is_cdrom
> +    drive_is_absolute_path

nit: should be sorted alphabetically

>      parse_drive
>      print_drive
>      storage_allows_io_uring_default
> @@ -913,7 +914,7 @@ PVE::JSONSchema::register_format(
>  sub verify_volume_id_or_absolute_path {
>      my ($volid, $noerr) = @_;
>  
> -    return $volid if $volid =~ m|^/|;
> +    return $volid if PVE::QemuServer::Helpers::is_absolute_path($volid);
>  
>      $volid = eval { PVE::JSONSchema::check_format('pve-volume-id', $volid, '') };
>      if ($@) {
> @@ -1555,7 +1556,7 @@ sub print_vga_device {
>  sub vm_is_volid_owner {
>      my ($storecfg, $vmid, $volid) = @_;
>  
> -    if ($volid !~ m|^/|) {
> +    if (!PVE::QemuServer::Helpers::is_absolute_path($volid)) {
>          my ($path, $owner);
>          eval { ($path, $owner) = PVE::Storage::path($storecfg, $volid); };
>          log_warn("ownership of volume '$volid' could not be determined: $@") if $@;
> @@ -1841,11 +1842,9 @@ sub destroy_vm {
>              { include_unused => 1 },
>              sub {
>                  my ($ds, $drive) = @_;
> -                return if drive_is_cdrom($drive);
> +                return if drive_is_cdrom($drive) || drive_is_absolute_path($drive);
>  
>                  my $volid = $drive->{file};
> -                return if !$volid || $volid =~ m|^/|;

Hm, the !$volid check should be included in the check above as well as
drive_is_absolute_path() doesn't check this and shouldn't need to.

Though if there's good reason that !$volid will never be true here, this
can be removed in a previous commit with a clear reasoning why.

> -
>                  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: $@")
> @@ -1859,10 +1858,9 @@ sub destroy_vm {
>      my $volids = {};
>      my $remove_owned_drive = sub {
>          my ($ds, $drive) = @_;
> -        return if drive_is_cdrom($drive, 1);
> +        return if drive_is_cdrom($drive, 1) || drive_is_absolute_path($drive);
>  
>          my $volid = $drive->{file};
> -        return if !$volid || $volid =~ m|^/|;
>          return if $volids->{$volid};

same here

>  
>          my ($path, $owner) = eval { PVE::Storage::path($storecfg, $volid) };
> @@ -6085,7 +6083,7 @@ sub get_vm_volumes {
>          sub {
>              my ($volid, $attr) = @_;
>  
> -            return if $volid =~ m|^/|;
> +            return if PVE::QemuServer::Helpers::is_absolute_path($volid);
>  
>              my ($sid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
>              return if !$sid;
> @@ -6485,7 +6483,7 @@ sub tar_restore_cleanup {
>              if ($line =~ m/vzdump:([^\s:]*):(\S+)$/) {
>                  my $volid = $2;
>                  eval {
> -                    if ($volid =~ m|^/|) {
> +                    if (PVE::QemuServer::Helpers::is_absolute_path($volid)) {
>                          unlink $volid || die 'unlink failed\n';
>                      } else {
>                          PVE::Storage::vdisk_free($storecfg, $volid);
> @@ -6530,11 +6528,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_is_absolute_path($drive);
>  
>              my $volid = $drive->{file};
> -            return if !$volid || $volid =~ m|^/|;

same here

> -
>              my ($path, $owner) = PVE::Storage::path($storecfg, $volid);
>              return if !$path || !$owner || ($owner != $vmid);
>  
> diff --git a/src/PVE/QemuServer/Blockdev.pm b/src/PVE/QemuServer/Blockdev.pm
> index 101c747c..78a53ff5 100644
> --- a/src/PVE/QemuServer/Blockdev.pm
> +++ b/src/PVE/QemuServer/Blockdev.pm
> @@ -11,7 +11,7 @@ use JSON;
>  use PVE::JSONSchema qw(json_bool);
>  use PVE::Storage;
>  
> -use PVE::QemuServer::Drive qw(drive_is_cdrom);
> +use PVE::QemuServer::Drive qw(drive_is_cdrom drive_is_absolute_path);

nit: should also be alphabetically sorted

>  use PVE::QemuServer::Helpers;
>  use PVE::QemuServer::Machine;
>  use PVE::QemuServer::Monitor qw(mon_cmd qmp_cmd qsd_qmp_peer vm_qmp_peer);
> @@ -299,7 +299,7 @@ sub generate_file_blockdev {
>      } elsif ($drive->{file} eq 'cdrom') {
>          my $path = PVE::QemuServer::Drive::get_iso_path($storecfg, $drive->{file});
>          $blockdev = { driver => 'host_cdrom', filename => "$path" };
> -    } elsif ($drive->{file} =~ m|^/|) {
> +    } elsif (drive_is_absolute_path($drive)) {
>          my $path = $drive->{file};
>          # The 'file' driver only works for regular files. The check below is taken from
>          # block/file-posix.c:hdev_probe_device() in QEMU. To detect CD-ROM host devices, QEMU issues
> diff --git a/src/PVE/QemuServer/Drive.pm b/src/PVE/QemuServer/Drive.pm
> index b80b7dbb..35256acf 100644
> --- a/src/PVE/QemuServer/Drive.pm
> +++ b/src/PVE/QemuServer/Drive.pm
> @@ -23,6 +23,7 @@ our @EXPORT_OK = qw(
>      checked_volume_format
>      drive_is_cloudinit
>      drive_is_cdrom
> +    drive_is_absolute_path

nit: should be sorted alphabetically

>      parse_drive
>      print_drive
>      storage_allows_io_uring_default
> @@ -766,6 +767,11 @@ sub drive_is_cdrom {
>      return $drive && $drive->{media} && ($drive->{media} eq 'cdrom');
>  }
>  
> +sub drive_is_absolute_path {
> +    my ($drive) = @_;
> +    return PVE::QemuServer::Helpers::is_absolute_path($drive->{file});
> +}
> +
>  sub parse_drive_interface {
>      my ($key) = @_;
>  
> @@ -1061,7 +1067,7 @@ sub get_scsi_device_type {
>      if (drive_is_cdrom($drive) || drive_is_cloudinit($drive)) {
>          $devicetype = 'cd';
>      } else {
> -        if ($drive->{file} =~ m|^/|) {
> +        if (drive_is_absolute_path($drive)) {
>              $path = $drive->{file};
>              if (my $info = path_is_scsi($path)) {
>                  if ($info->{type} == 0 && $drive->{scsiblock}) {
> diff --git a/src/PVE/QemuServer/Helpers.pm b/src/PVE/QemuServer/Helpers.pm
> index dd17eef5..5cc3e8b5 100644
> --- a/src/PVE/QemuServer/Helpers.pm
> +++ b/src/PVE/QemuServer/Helpers.pm
> @@ -389,4 +389,9 @@ sub get_host_phys_address_bits {
>      return; # undef, cannot really do anything..
>  }
>  
> +sub is_absolute_path {

the subroutine name is rather short in description, this should be at
least

    volid_is_absolute_path()

or

    volumeid_is_absolute_path()

or something different.

> +    my ($volid) = @_;
> +    return $volid =~ m|^/|;
> +}
> +
>  1;