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

["Max R. Carrara" <m.carrara@proxmox.com>]

On Tue Jul 28, 2026 at 4:29 PM CEST, Thomas Ellmenreich wrote:
> 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.

Oh yeah, that would of course be a good spot to place them! I hadn't
checked for any existing places 😇

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

I'm very glad! If you have any more questions etc. please let me know,
I'm always happy to help :)

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