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

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

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.

> - 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.

>
> - 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.

>   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 don't really see a use case for it in pve-storage. Then, use a
>   separate subroutine for each check. Once we see a need for these
>   helpers elsewhere, we can start generalizing them, maybe even move
>   them to pve-common or something similar. The existing subroutines in
>   qemu-server could then just be wrappers for those in pve-common (or
>   some other package).
>
>   That way you can do your refactor in qemu-server and then later expand
>   to other places, without having to worry about additional churn, or
>   having to move constants around.
>
> I hope all of this makes sense -- I know it's a lot, but I wanted to
> share all of my thoughts here, esp. because I have stepped into many
> (way too many) little traps myself when it comes to refactoring Perl
> code. I do like the fact that you're tackling this a lot; I'm always a
> fan of reducing the number of random regexes and strings that are
> floating around. So, I hope my comments are of use to you! :)

The comments are very much appreciated ^^

>
> [series] https://lore.proxmox.com/pve-devel/20260422111322.257380-1-m.carrara@proxmox.com/