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

["Thomas Ellmenreich" <t.ellmenreich@proxmox.com>]

Thank you for having tested this!

On Thu Aug 13, 2026 at 12:50 PM CEST, Elias Huhsovitz wrote:
> Testing
> -------
> I tested this using qm create with all affected drive types, e.g.,
> qm create <vmid> 
> --ide2 none,media=crom | cdrom | etc 
> --scsi0 local-lvm:5 | etc
>
> and then destroying them using
> qm destroy <vmid> --purge
>
> It worked as intended.
>
> Small note
> ----------
> Small note inline, but no change necessary IMHO.
> Therefore:
>
> Reviewed-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
> Tested-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>

[snip]

>> +# Tries to classify the drive file as either 'none', 'cdrom', 'absolute'
>> +# or 'volume'. In all other cases it will return 'unknown'.
>> +sub classify_drive_file {
>> +    my ($volid) = @_;
>> +
>> +    return 'unknown' if !$volid;
>> +
>> +    if ($volid eq 'none') {
>> +        return 'none';
>> +    }
>> +
>> +    if ($volid eq 'cdrom') {
>> +        return 'cdrom';
>> +    }
>> +
>> +    if ($volid =~ m|^/|) {
>> +        return 'absolute';
>> +    }
>> +
>> +    return PVE::Storage::parse_volume_id($volid, 1)
>> +        ? 'volume'
>> +        : 'unknown';
>> +}
>
> I think the previous if-else based approach was fine here, but this is
> also good. I just noticed one thing regarding this approach. Since we
> are no longer mimicking a switch-case statement we could combine the the
> 'none' and 'cdrom' cases into one like this:

I changed the if statements based on this comment [1], by @Max, which I do
agree with. The ifelse chain is more packed and not as clear or modifiable as
a chain of early returns.

[1]: https://lore.proxmox.com/pve-devel/DKILUOJ6LFZ7.1X2WY1PSWHQYG@proxmox.com/

> if ($volid eq 'none' || $volid eq 'cdrom') {
>         return $volid;
> }
>
> But i think the current approach is good and you don't need to change this!

I actually also considered using this approach, but in the end decided against
it, as I preferred having all of the possible return values written down
instead of just returning the variable. (Which functionally is obviously the
same thing).

[snip]