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

[Fiona Ebner <f.ebner@proxmox.com>]

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.