Re: [PATCH] fix #7828: avoid parsing absolute paths as volume ids

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

Thanks for sending in a patch for this!

On Tue Jul 21, 2026 at 2:20 PM 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.
>
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>

This should have a

Fixes: 5b5c5768 ("display warnings for storage errors or if storage no longer exists")

as the added PVE::Storage::parse_volume_id() in that commit introduced
this issue as it assumed that there would never be a absolute path
storage here in $drive->{file}.

> ---
> I'm not 100% sure that I have understood the procedure of registering
> unused drives correctly, but skipping this branch in the case of an
> absolute path as 'volid' seems to be the correct action.
>
> Alternatively, placing the condition next to the `storage_config` check
> could make sense, but would just achieve the same thing, as the
> subsequent ownership check will never succeed with an absolute path.

I think the placement is fine here.

I think it would be nice to add a patch before this to add a helper
subroutine PVE::QemuServer::Drive::drive_is_absolute_path() or something
similar:

sub drive_is_absolute_path {
    my ($drive) = @_;
    return $drive->{file} =~ m|^/|;
}

and replace all current checks =~ m|^/| and !~ m|^/| appropriately in
qemu-server similar to the drive_is_cdrom() and drive_is_cloudinit() to
make it easier to understand the special case here.

>
>  src/PVE/QemuServer.pm | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 191ae549..782d2e7b 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -1570,12 +1570,14 @@ sub vm_is_volid_owner {
>  sub vmconfig_register_unused_drive {
>      my ($storecfg, $vmid, $conf, $drive) = @_;
>  
> +    my $volid = $drive->{file};
> +
>      if (drive_is_cloudinit($drive)) {
> -        eval { PVE::Storage::vdisk_free($storecfg, $drive->{file}) };
> +        eval { PVE::Storage::vdisk_free($storecfg, $volid) };

It is best to do such changes in their own patch either preceding this
or succeding this patch to make the fixing patch only about fixing the
patch and nothing else ;).

>          warn $@ if $@;
>          delete $conf->{'special-sections'}->{cloudinit};
> -    } elsif (!drive_is_cdrom($drive)) {
> -        my $volid = $drive->{file};
> +    } elsif (!drive_is_cdrom($drive) && $volid !~ m|^/|) {
> +
>          my ($storeid, undef) = PVE::Storage::parse_volume_id($volid);
>          if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {
>              if (vm_is_volid_owner($storecfg, $vmid, $volid)) {