Re: [RFC qemu-server/storage 0/2] refactor of volume_id classification

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

Am 28.07.26 um 4:28 PM schrieb Thomas Ellmenreich:
> First of all, thanks for taking a look ;)
> 
> Some comments below. I will wait a little longer to see if anyone else
> would like to leave some comments, but then I will send a v1.
> 
> On Tue Jul 28, 2026 at 3:30 PM CEST, Max R. Carrara wrote:
> 
> [snip]
> 
>> Overall I like this idea a lot, but there are some things that IMO still
>> need to be fleshed out more. A bunch of comments:
>>
>> - I'm personally not a fan of `use constant` anymore, since it's rather
>>   unwieldy and doesn't play too nice with the rest of Perl (e.g.
>>   interpolating those constants is annoying).
> 
> good to know
> 
>>
>> - I would also refrain from exposing any additional constants / variables
>>   in PVE::Storage or PVE::Storage::Plugin, because those things
>>   implicitly become part of the public API of PVE::Storage, which in
>>   turn means it's really hard to change them later on.
> 
> also good to know
> 
>>   In fact, in one of my recent [series] I'm actively trying to get rid
>>   of the `our $RE_.*` regexes in PVE::Storage for precisely that reason
>>   (see patches #22 - #28 there).
>>
>>   Perl's lack of a type system also makes it rather annoying to model
>>   such things -- in Rust this would be just a neat enum and we could
>>   call it a day.
> 
> Yes, that's what I was also thinking of, but perl does not make it easy.
> 

I'm also not sure if we should go out of our way trying to model an
actual type system in Perl. I think using strings in place of constants
is fine in this case.

>> - The other thing is, do we have a need for anything but 'volume' in
>>   pve-storage? Wouldn't it make sense for this helper to live somewhere
>>   else?
>>
>>   If we decide to keep this in pve-storage, it should only remain in
>>   PVE::Storage or PVE::Storage::Common, and not PVE::Storage::Plugin,
>>   since ::Plugin should only contain things relevant for the storage
>>   plugin API.
> 
> Yeah, I think you are right with your assumption. I was also considering
> just leaving the subroutine in 'qemu_server', but then I found it fitting
> to place it next to 'parse_volume_id'. That said, 'qemu_server' is the
> better location, so I will place it there.

Yes, I fully agree. It classifies a pve-volume-id-or-qm-path which the
storage layer has no idea about ;)

> 
>>
>> - Also, such functions should not throw an error at all -- you have
>>   noticed yourself that you always pass `$noerr`, so there's most likely
>>   not a need for it to exist. If the caller throws an error directly
>>   if something doesn't match, it's easier to see what the intention
>>   behind the check is.
> 
> :thumbs-up:
> 
>>
>> - So, with all that being said, I instead suggest using four subroutines
>>   that each perform their own dedicated check, e.g.:
>>
>>   use v5.36;
>>
>>   use Exporter qw(import);
>>
>>   our @EXPORT_OK = qw(
>>       is_cdrom
>>   );
>>
>>   sub is_cdrom : prototype($) ($value) {
>>       return defined($value) && $value eq 'cdrom';
>>   }
>>
>>   ... and so on.
>>
>>   Without checking in greater detail, these helper functions can
>>   probably live in their own module somewhere inside qemu-server, though
>>   I don't know where else you are planning to use these helpers.
>>
>>   While it would probably be considered insane to use a separate
>>   function for each check in a normal programming language, Perl doesn't
>>   really provide any (sane) constructs (without 10.000 gotchas) that
>>   allow you to express this kind of thing in a neater manner.
> 
> I was actually also considering this option, but then discarded it in favour
> of the constants. Since those are not an option anymore, I don't find this
> approach that bad.

Not a fan of this idea. For one, we already have a drive_is_cdrom() and
this would be way too easy to confuse with that. But it's also just
unnecessary complexity. Just do it like classify_mountpoint(), it's
simple, straightforward and does what it says.

> 
>>   Separate subroutines also make it easier to change the underlying
>>   implementation once necessary -- I doubt this will matter much here,
>>   since the checks are relatively simple, but if we e.g. use perlmod
>>   more and end up expressing this as an enum in Rust, then we could let
>>   those subroutines wrap the (single) function that we expose through
>>   perlmod instead. If we were to introduce new constants in Perl, this
>>   would not be possible -- you would have to modify each call site where
>>   those constants are used again.
>>
>>   This is just an example, but I hope this makes sense.
>>
>> - To summarize all of my rambling above, I would suggest putting this
>>   into a dedicated module for such utils somewhere in qemu-server, as
> 
> Should I really create a completely new module? I was thinking of placing them
> in PVE::QemuServer::Helpers, but maybe that is not quite correct? And the
> names would have to be a bit more explicit if they were placed in ::Helpers.

I feel like it belongs into the QemuServer::Drive module. The
verify_volume_id_or_qm_path and format registration for it could also
move there.