* [PATCH qemu-server v2 0/2] fix #7828: avoid parsing absolute paths as volume ids.
@ 2026-07-22 9:52 Thomas Ellmenreich
2026-07-22 9:52 ` [PATCH qemu-server v2 1/2] refactor checks for absolute path as drive Thomas Ellmenreich
2026-07-22 9:52 ` [PATCH qemu-server v2 2/2] fix #7828: avoid parsing absolute paths as volume ids Thomas Ellmenreich
0 siblings, 2 replies; 9+ messages in thread
From: Thomas Ellmenreich @ 2026-07-22 9:52 UTC (permalink / raw)
To: pve-devel; +Cc: Thomas Ellmenreich
This series refactors all checks of drive and volume ids as absolute paths
into two separate helper functions, and uses them in all occurrences of such
checks.
The new helper functions are then also applied to make sure that drives that
are absolute paths are not added as unused drives to a vm. Just like is the
case for cdrom drives.
This issue was reported in the community forum: [0]. The issue arose, as
@Daniel Kral mentioned, because of the addition of the config check and parsing
of the volume id with commit: 5b5c5768, which did not consider the possibility
of absolute paths as volume ids.
Changes since v1:
- Prepended an additional patch to introduce the two helper functions, as the
check for absolute paths was performed a considerable amount of times.
- Use of the created helper functions:
- Addition of the "Fixes 5b..." to the commit message
[0]: https://forum.proxmox.com/threads/possible-regression-in-pve-9-2-x-unable-to-remove-physical-passthrough-disks-using-qm-set-delete.185191/
qemu-manager:
Thomas Ellmenreich (2):
refactor checks for absolute path as drive
fix #7828: avoid parsing absolute paths as volume ids
src/PVE/QemuConfig.pm | 2 +-
src/PVE/QemuMigrate.pm | 2 +-
src/PVE/QemuServer.pm | 22 +++++++++-------------
src/PVE/QemuServer/Blockdev.pm | 4 ++--
src/PVE/QemuServer/Drive.pm | 8 +++++++-
src/PVE/QemuServer/Helpers.pm | 5 +++++
6 files changed, 25 insertions(+), 18 deletions(-)
Summary over all repositories:
6 files changed, 25 insertions(+), 18 deletions(-)
--
Generated by murpp 0.12.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH qemu-server v2 1/2] refactor checks for absolute path as drive
2026-07-22 9:52 [PATCH qemu-server v2 0/2] fix #7828: avoid parsing absolute paths as volume ids Thomas Ellmenreich
@ 2026-07-22 9:52 ` Thomas Ellmenreich
2026-07-22 12:45 ` Daniel Kral
2026-07-22 9:52 ` [PATCH qemu-server v2 2/2] fix #7828: avoid parsing absolute paths as volume ids Thomas Ellmenreich
1 sibling, 1 reply; 9+ messages in thread
From: Thomas Ellmenreich @ 2026-07-22 9:52 UTC (permalink / raw)
To: pve-devel; +Cc: Thomas Ellmenreich
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(-)
diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm
index 26f0fda2..d8733d9d 100644
--- a/src/PVE/QemuConfig.pm
+++ b/src/PVE/QemuConfig.pm
@@ -145,7 +145,7 @@ sub get_replicatable_volumes {
return if !$cleanup && !$attr->{replicate};
- if ($volid =~ m|^/|) {
+ if (PVE::QemuServer::Helpers::is_absolute_path($volid)) {
return if !$attr->{replicate};
return if $cleanup || $noerr;
die "unable to replicate local file/device '$volid'\n";
diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm
index 8da6f15d..dcf911a5 100644
--- a/src/PVE/QemuMigrate.pm
+++ b/src/PVE/QemuMigrate.pm
@@ -405,7 +405,7 @@ sub scan_local_volumes {
my $test_volid = sub {
my ($volid, $attr) = @_;
- if ($volid =~ m|^/|) {
+ if (PVE::QemuServer::Helpers::is_absolute_path($volid)) {
return if $attr->{shared};
$local_volumes->{$volid}->{ref} = 'config';
die "local file/device\n";
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
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|^/|;
-
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};
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|^/|;
-
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);
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
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 {
+ my ($volid) = @_;
+ return $volid =~ m|^/|;
+}
+
1;
--
2.47.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH qemu-server v2 2/2] fix #7828: avoid parsing absolute paths as volume ids
2026-07-22 9:52 [PATCH qemu-server v2 0/2] fix #7828: avoid parsing absolute paths as volume ids Thomas Ellmenreich
2026-07-22 9:52 ` [PATCH qemu-server v2 1/2] refactor checks for absolute path as drive Thomas Ellmenreich
@ 2026-07-22 9:52 ` Thomas Ellmenreich
2026-07-22 13:01 ` Fiona Ebner
2026-07-22 14:10 ` Elias Huhsovitz
1 sibling, 2 replies; 9+ messages in thread
From: Thomas Ellmenreich @ 2026-07-22 9:52 UTC (permalink / raw)
To: pve-devel; +Cc: Thomas Ellmenreich
Just like in the case of cdroms, absolute paths to storage drives can be
skipped when beeing marked as unused as they cannot be owned by the vm.
Fixes: 5b5c5768 ("display warnings for storage errors or if storage no longer exists")
Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
src/PVE/QemuServer.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index ce832c8e..ce0eda74 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -1575,7 +1575,7 @@ sub vmconfig_register_unused_drive {
eval { PVE::Storage::vdisk_free($storecfg, $drive->{file}) };
warn $@ if $@;
delete $conf->{'special-sections'}->{cloudinit};
- } elsif (!drive_is_cdrom($drive)) {
+ } elsif (!drive_is_cdrom($drive) && !drive_is_absolute_path($drive)) {
my $volid = $drive->{file};
my ($storeid, undef) = PVE::Storage::parse_volume_id($volid);
if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {
--
2.47.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH qemu-server v2 1/2] refactor checks for absolute path as drive
2026-07-22 9:52 ` [PATCH qemu-server v2 1/2] refactor checks for absolute path as drive Thomas Ellmenreich
@ 2026-07-22 12:45 ` Daniel Kral
2026-07-22 13:40 ` Thomas Ellmenreich
2026-07-22 14:01 ` Fiona Ebner
0 siblings, 2 replies; 9+ messages in thread
From: Daniel Kral @ 2026-07-22 12:45 UTC (permalink / raw)
To: Thomas Ellmenreich, pve-devel
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;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH qemu-server v2 2/2] fix #7828: avoid parsing absolute paths as volume ids
2026-07-22 9:52 ` [PATCH qemu-server v2 2/2] fix #7828: avoid parsing absolute paths as volume ids Thomas Ellmenreich
@ 2026-07-22 13:01 ` Fiona Ebner
2026-07-22 13:11 ` Thomas Ellmenreich
2026-07-22 14:10 ` Elias Huhsovitz
1 sibling, 1 reply; 9+ messages in thread
From: Fiona Ebner @ 2026-07-22 13:01 UTC (permalink / raw)
To: Thomas Ellmenreich, pve-devel
Am 22.07.26 um 11:53 AM schrieb Thomas Ellmenreich:
> Just like in the case of cdroms, absolute paths to storage drives can be
> skipped when beeing marked as unused as they cannot be owned by the vm.
>
> Fixes: 5b5c5768 ("display warnings for storage errors or if storage no longer exists")
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
> src/PVE/QemuServer.pm | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index ce832c8e..ce0eda74 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -1575,7 +1575,7 @@ sub vmconfig_register_unused_drive {
> eval { PVE::Storage::vdisk_free($storecfg, $drive->{file}) };
> warn $@ if $@;
> delete $conf->{'special-sections'}->{cloudinit};
> - } elsif (!drive_is_cdrom($drive)) {
> + } elsif (!drive_is_cdrom($drive) && !drive_is_absolute_path($drive)) {
> my $volid = $drive->{file};
> my ($storeid, undef) = PVE::Storage::parse_volume_id($volid);
To fix the regression, I would prefer to use the noerr variant of
parse_volume_id() here, because that is what tells us whether the volume
is PVE-managed or not. We already do that in some other places like
import. And then continue only if we got a result for $storeid.
> if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH qemu-server v2 2/2] fix #7828: avoid parsing absolute paths as volume ids
2026-07-22 13:01 ` Fiona Ebner
@ 2026-07-22 13:11 ` Thomas Ellmenreich
0 siblings, 0 replies; 9+ messages in thread
From: Thomas Ellmenreich @ 2026-07-22 13:11 UTC (permalink / raw)
To: Fiona Ebner, pve-devel, d.kral
> To fix the regression, I would prefer to use the noerr variant of
> parse_volume_id() here, because that is what tells us whether the volume
> is PVE-managed or not. We already do that in some other places like
> import. And then continue only if we got a result for $storeid.
That sounds like a better solution for me, what do you think @Daniel?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH qemu-server v2 1/2] refactor checks for absolute path as drive
2026-07-22 12:45 ` Daniel Kral
@ 2026-07-22 13:40 ` Thomas Ellmenreich
2026-07-22 14:01 ` Fiona Ebner
1 sibling, 0 replies; 9+ messages in thread
From: Thomas Ellmenreich @ 2026-07-22 13:40 UTC (permalink / raw)
To: Daniel Kral, pve-devel
On Wed Jul 22, 2026 at 2:45 PM CEST, Daniel Kral wrote:
[snip]
> 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.
Good catch, was a thinking mistake on my part. Thought the `!volid`
check was unnecessary because the regex cleanly handles it, although
that misses the point. Will reintroduce it.
[snip]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH qemu-server v2 1/2] refactor checks for absolute path as drive
2026-07-22 12:45 ` Daniel Kral
2026-07-22 13:40 ` Thomas Ellmenreich
@ 2026-07-22 14:01 ` Fiona Ebner
1 sibling, 0 replies; 9+ messages in thread
From: Fiona Ebner @ 2026-07-22 14:01 UTC (permalink / raw)
To: Daniel Kral, Thomas Ellmenreich, pve-devel
Am 22.07.26 um 2:45 PM schrieb Daniel Kral:
> 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.
While these helpers are fine at a glance, a classify_drive_volid()
similar to LXC/Config.pm's classify_mountpoint() might be a bit nicer in
the long run. It could return 'absolute', 'none', 'cdrom' and 'volume'.
>> @@ -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.
We should be protected. We don't allow empty values as part of a
property string and even if, the schema validation against
'pve-volume-id-or-qm-path' would fail. The implementation in
foreach_volume_full() in pve-guest-common won't call the closure if
parsing fails. So I think dropping the check is fine. If it really
triggers, it will just result in a warning pointing to a programming
error somewhere else, which we should fix then.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH qemu-server v2 2/2] fix #7828: avoid parsing absolute paths as volume ids
2026-07-22 9:52 ` [PATCH qemu-server v2 2/2] fix #7828: avoid parsing absolute paths as volume ids Thomas Ellmenreich
2026-07-22 13:01 ` Fiona Ebner
@ 2026-07-22 14:10 ` Elias Huhsovitz
1 sibling, 0 replies; 9+ messages in thread
From: Elias Huhsovitz @ 2026-07-22 14:10 UTC (permalink / raw)
To: Thomas Ellmenreich, pve-devel
Small typo, otherwise see review by fiona Ebner [1].
[1:] https://lore.proxmox.com/all/8abb29af-e16d-456b-ab14-cd423abdd801@proxmox.com/
On Wed Jul 22, 2026 at 11:52 AM CEST, Thomas Ellmenreich wrote:
> Just like in the case of cdroms, absolute paths to storage drives can be
> skipped when beeing marked as unused as they cannot be owned by the vm.
^^^
small typo
>
> Fixes: 5b5c5768 ("display warnings for storage errors or if storage no longer exists")
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
> src/PVE/QemuServer.pm | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index ce832c8e..ce0eda74 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -1575,7 +1575,7 @@ sub vmconfig_register_unused_drive {
> eval { PVE::Storage::vdisk_free($storecfg, $drive->{file}) };
> warn $@ if $@;
> delete $conf->{'special-sections'}->{cloudinit};
> - } elsif (!drive_is_cdrom($drive)) {
> + } elsif (!drive_is_cdrom($drive) && !drive_is_absolute_path($drive)) {
> my $volid = $drive->{file};
> my ($storeid, undef) = PVE::Storage::parse_volume_id($volid);
> if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-07-22 14:10 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-22 9:52 [PATCH qemu-server v2 0/2] fix #7828: avoid parsing absolute paths as volume ids Thomas Ellmenreich
2026-07-22 9:52 ` [PATCH qemu-server v2 1/2] refactor checks for absolute path as drive Thomas Ellmenreich
2026-07-22 12:45 ` Daniel Kral
2026-07-22 13:40 ` Thomas Ellmenreich
2026-07-22 14:01 ` Fiona Ebner
2026-07-22 9:52 ` [PATCH qemu-server v2 2/2] fix #7828: avoid parsing absolute paths as volume ids Thomas Ellmenreich
2026-07-22 13:01 ` Fiona Ebner
2026-07-22 13:11 ` Thomas Ellmenreich
2026-07-22 14:10 ` Elias Huhsovitz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.