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 2:22 PM CEST, Thomas Ellmenreich wrote:
> Refactor of volume_id classification
> ====================================
>
> While developing this: [0] patch series, I refactored all the checks for
> volume id's as absolute paths into their own subroutine. The original series
> was then dropped in favour of a quicker fix: [1]. This series revives the
> refactor without the fix and, unlike the original series creates a new
> classify_volume_id subroutine, as @Fiona Ebener mentioned here: [2].
>
> Overview
> --------
>
> As previously mentioned, a new classify_volume_id subroutine has been
> introduced, which can then be used to determine the type of volume id. This
> type can be one of:
>
> > 'none', 'cdrom', 'absolute', 'volume'
>
> The subroutine and constants are exported by the pve-storage package and
> used in the qemu-server package.
>
> Open Questions
> --------------
>
> - The classify subroutine currently throws an error if the volume id does not
>   match any of the patterns (when 'noerr' is not selected). I'm not sure if
>   this is the correct way to implement a classification method, given that all
>   my uses enable the noerr option. That said, the case where an error is thrown
>   should be very unlikely and would point to an error that should be fixed.
>
> - I have decided to only use the new function in the QemuServer.qm module,
>   since other uses can then be added incrementally. Does this approach make
>   sense?
>
> [0]: https://lore.proxmox.com/pve-devel/20260722095251.89606-1-t.ellmenreich@proxmox.com/T/#t
> [1]: https://lore.proxmox.com/pve-devel/20260724081611.11254-1-f.ebner@proxmox.com/
> [2]: https://lore.proxmox.com/pve-devel/0b4272e1-3634-4fdd-b79f-6b077dd387d5@proxmox.com/
>
> [...]

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

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

  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.

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

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

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

  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
  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! :)

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