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

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

[Thomas Ellmenreich]

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/


pve-storage:

Thomas Ellmenreich (1):
  add subroutine to classify volume ids

 src/PVE/Storage.pm        | 21 +++++++++++++++++++++
 src/PVE/Storage/Plugin.pm | 25 +++++++++++++++++++++++++
 2 files changed, 46 insertions(+)


qemu-manager:

Thomas Ellmenreich (1):
  use new classify_volume_id subroutine

 src/PVE/QemuServer.pm       | 39 ++++++++++++++++++++++++-------------
 src/PVE/QemuServer/Drive.pm |  6 ++++++
 2 files changed, 32 insertions(+), 13 deletions(-)


Summary over all repositories:
  4 files changed, 78 insertions(+), 13 deletions(-)

-- 
Generated by murpp 0.12.0

[PATCH storage 1/2] add subroutine to classify volume ids

[Thomas Ellmenreich]

To enable easier branching on different types of volume id's a new
subroutine classify_volume_id is added. It classifies the passed type
as one of 'volume', 'cdrom', 'none' or 'absolute'. Depending on how
the 'noerr' option is set values that cannot be categorized throw an
error or return undef.

The constants and subroutine are exported by the Storage module

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 src/PVE/Storage.pm        | 21 +++++++++++++++++++++
 src/PVE/Storage/Plugin.pm | 25 +++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 64ea9da..1401d69 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -40,6 +40,16 @@ use PVE::Storage::PBSPlugin;
 use PVE::Storage::BTRFSPlugin;
 use PVE::Storage::ESXiPlugin;
 
+use base qw(Exporter);
+
+our @EXPORT_OK = qw(
+    classify_volume_id
+    $VOL_NONE
+    $VOL_CDROM
+    $VOL_ABSOLUTE
+    $VOL_VOLUME
+);
+
 # Storage API version. Increment it on changes in storage API interface.
 use constant APIVER => 15;
 # Age is the number of versions we're backward compatible with.
@@ -131,6 +141,11 @@ our $OVA_CONTENT_RE_1 = qr/${SAFE_CHAR_WITH_WHITESPACE_CLASS_RE}+\.(qcow2|raw|vm
 # FIXME remove with PVE 9.0, add versioned breaks for pve-manager
 our $vztmpl_extension_re = $VZTMPL_EXT_RE_1;
 
+our $VOL_NONE = PVE::Storage::Plugin::VOL_NONE();
+our $VOL_CDROM = PVE::Storage::Plugin::VOL_CDROM();
+our $VOL_ABSOLUTE = PVE::Storage::Plugin::VOL_ABSOLUTE();
+our $VOL_VOLUME = PVE::Storage::Plugin::VOL_VOLUME();
+
 # See the QMP reference documentation.
 my $allowed_qemu_blockdev_options_file = {
     filename => 1,
@@ -615,6 +630,12 @@ sub parse_volume_id {
     return PVE::Storage::Plugin::parse_volume_id($volid, $noerr);
 }
 
+sub classify_volume_id {
+    my ($volid, $noerr) = @_;
+
+    return PVE::Storage::Plugin::classify_volume_id($volid, $noerr);
+}
+
 # test if we have read access to volid
 sub check_volume_access {
     my ($rpcenv, $user, $cfg, $vmid, $volid, $type) = @_;
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 4f69f9b..180ad6c 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -27,6 +27,14 @@ use constant COMPRESSOR_RE => join('|', KNOWN_COMPRESSION_FORMATS);
 use constant LOG_EXT => ".log";
 use constant NOTES_EXT => ".notes";
 
+# types of volume ids
+use constant {
+    VOL_NONE => 'none',
+    VOL_CDROM => 'cdrom',
+    VOL_ABSOLUTE => 'absolute',
+    VOL_VOLUME => 'volume',
+};
+
 our @COMMON_TAR_FLAGS = qw(
     --one-file-system
     -p --sparse --numeric-owner --acls
@@ -427,6 +435,23 @@ sub parse_volume_id {
     die "unable to parse volume ID '$volid'\n";
 }
 
+sub classify_volume_id {
+    my ($volid, $noerr) = @_;
+
+    if (!defined($volid) || $volid eq "none") {
+        return VOL_NONE;
+    } elsif ($volid eq 'cdrom') {
+        return VOL_CDROM;
+    } elsif ($volid =~ m|^/|) {
+        return VOL_ABSOLUTE;
+    } elsif (parse_volume_id($volid, 1)) {
+        return VOL_VOLUME;
+    }
+
+    return undef if $noerr;
+    die "unable to classify volume ID '$volid'";
+}
+
 PVE::JSONSchema::register_format('pve-dir-override', \&verify_dir_override);
 
 sub verify_dir_override {
-- 
2.47.3

[PATCH qemu-server 2/2] use new classify_volume_id subroutine

[Thomas Ellmenreich]

refactors checks of volume ids for different patterns to use the new
classify_volume_id subroutine exposed by pve_storage.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 src/PVE/QemuServer.pm       | 39 ++++++++++++++++++++++++-------------
 src/PVE/QemuServer/Drive.pm |  6 ++++++
 2 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 9aec7f9c..33176ab5 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -43,7 +43,12 @@ use PVE::PBSClient;
 use PVE::RESTEnvironment qw(log_warn);
 use PVE::RPCEnvironment;
 use PVE::SafeSyslog;
-use PVE::Storage;
+use PVE::Storage qw(
+    $VOL_ABSOLUTE
+    $VOL_CDROM
+    $VOL_NONE
+    $VOL_VOLUME
+);
 use PVE::SysFSTools;
 use PVE::Systemd;
 use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_foreach $IPV6RE);
@@ -73,6 +78,7 @@ use PVE::QemuServer::CPUFlags;
 use PVE::QemuServer::Drive qw(
     is_valid_drivename
     checked_volume_format
+    drive_is_absolute_path
     drive_is_cloudinit
     drive_is_cdrom
     parse_drive
@@ -900,7 +906,8 @@ PVE::JSONSchema::register_format('pve-volume-id-or-qm-path', \&verify_volume_id_
 sub verify_volume_id_or_qm_path {
     my ($volid, $noerr) = @_;
 
-    return $volid if $volid eq 'none' || $volid eq 'cdrom';
+    my $vol_type = PVE::Storage::classify_volume_id($volid, 1);
+    return $volid if $vol_type eq $VOL_NONE || $vol_type eq $VOL_CDROM;
 
     return verify_volume_id_or_absolute_path($volid, $noerr);
 }
@@ -913,7 +920,7 @@ PVE::JSONSchema::register_format(
 sub verify_volume_id_or_absolute_path {
     my ($volid, $noerr) = @_;
 
-    return $volid if $volid =~ m|^/|;
+    return $volid if PVE::Storage::classify_volume_id($volid, 1) eq $VOL_ABSOLUTE;
 
     $volid = eval { PVE::JSONSchema::check_format('pve-volume-id', $volid, '') };
     if ($@) {
@@ -1130,7 +1137,10 @@ sub cleanup_drive_path {
         $drive->{file} = $volid;
     }
 
-    $drive->{media} = 'cdrom' if !$drive->{media} && $drive->{file} =~ m/^(cdrom|none)$/;
+    my $vol_type = PVE::Storage::classify_volume_id($drive->{file}, 1);
+
+    $drive->{media} = 'cdrom'
+        if !$drive->{media} && ($vol_type eq $VOL_CDROM || $vol_type eq $VOL_NONE);
 }
 
 sub parse_hotplug_features {
@@ -1555,7 +1565,7 @@ sub print_vga_device {
 sub vm_is_volid_owner {
     my ($storecfg, $vmid, $volid) = @_;
 
-    if ($volid !~ m|^/|) {
+    if (PVE::Storage::classify_volume_id($volid, 1) ne $VOL_ABSOLUTE) {
         my ($path, $owner);
         eval { ($path, $owner) = PVE::Storage::path($storecfg, $volid); };
         log_warn("ownership of volume '$volid' could not be determined: $@") if $@;
@@ -1845,7 +1855,7 @@ sub destroy_vm {
                 return if drive_is_cdrom($drive);
 
                 my $volid = $drive->{file};
-                return if !$volid || $volid =~ m|^/|;
+                return if !$volid || drive_is_absolute_path($drive);
 
                 my $result = eval { PVE::Storage::volume_is_base_and_used($storecfg, $volid) };
                 # early check, removal of volume will fail later anyway, so warning here is fine
@@ -1863,7 +1873,7 @@ sub destroy_vm {
         return if drive_is_cdrom($drive, 1);
 
         my $volid = $drive->{file};
-        return if !$volid || $volid =~ m|^/|;
+        return if !$volid || drive_is_absolute_path($drive);
         return if $volids->{$volid};
 
         my ($path, $owner) = eval { PVE::Storage::path($storecfg, $volid) };
@@ -3625,7 +3635,7 @@ sub config_to_command {
             my $live_restore = $live_restore_backing->{$ds};
 
             if (min_version($machine_version, 10, 0)) { # for the switch to -blockdev
-                if ($drive->{file} ne 'none') {
+                if (PVE::Storage::classify_volume_id($drive->{file}, 1) ne $VOL_NONE) {
                     my $throttle_group =
                         PVE::QemuServer::Blockdev::generate_throttle_group($drive);
                     push @$cmd, '-object', to_json($throttle_group, { canonical => 1 });
@@ -5304,7 +5314,10 @@ sub vmconfig_update_disk {
             eval { PVE::QemuServer::Blockdev::change_medium($storecfg, $vmid, $opt, $drive); };
             my $err = $@;
 
-            if ($drive->{file} eq 'none' && drive_is_cloudinit($old_drive)) {
+            if (
+                PVE::Storage::classify_volume_id($drive->{file}, 1) eq $VOL_NONE
+                && drive_is_cloudinit($old_drive)
+            ) {
                 vmconfig_register_unused_drive($storecfg, $vmid, $conf, $old_drive);
             }
 
@@ -6086,7 +6099,7 @@ sub get_vm_volumes {
         sub {
             my ($volid, $attr) = @_;
 
-            return if $volid =~ m|^/|;
+            return if PVE::Storage::classify_volume_id($volid, 1) eq $VOL_ABSOLUTE;
 
             my ($sid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
             return if !$sid;
@@ -6110,7 +6123,7 @@ sub get_current_vm_volumes {
         sub {
             my ($ds, $drive) = @_;
 
-            if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
+            if (PVE::Storage::classify_volume_id($drive->{file}, 1) eq $VOL_VOLUME) {
                 check_volume_storage_type($storecfg, $drive->{file});
                 push $volumes->@*, $drive->{file};
             }
@@ -6486,7 +6499,7 @@ sub tar_restore_cleanup {
             if ($line =~ m/vzdump:([^\s:]*):(\S+)$/) {
                 my $volid = $2;
                 eval {
-                    if ($volid =~ m|^/|) {
+                    if (PVE::Storage::classify_volume_id($volid, 1) eq $VOL_ABSOLUTE) {
                         unlink $volid || die 'unlink failed\n';
                     } else {
                         PVE::Storage::vdisk_free($storecfg, $volid);
@@ -6534,7 +6547,7 @@ my $restore_cleanup_oldconf = sub {
             return if drive_is_cdrom($drive, 1);
 
             my $volid = $drive->{file};
-            return if !$volid || $volid =~ m|^/|;
+            return if !$volid || drive_is_absolute_path($drive);
 
             my ($path, $owner) = PVE::Storage::path($storecfg, $volid);
             return if !$path || !$owner || ($owner != $vmid);
diff --git a/src/PVE/QemuServer/Drive.pm b/src/PVE/QemuServer/Drive.pm
index b80b7dbb..5738c0bc 100644
--- a/src/PVE/QemuServer/Drive.pm
+++ b/src/PVE/QemuServer/Drive.pm
@@ -21,6 +21,7 @@ our @EXPORT_OK = qw(
     is_valid_drivename
     checked_parse_volname
     checked_volume_format
+    drive_is_absolute_path
     drive_is_cloudinit
     drive_is_cdrom
     parse_drive
@@ -766,6 +767,11 @@ sub drive_is_cdrom {
     return $drive && $drive->{media} && ($drive->{media} eq 'cdrom');
 }
 
+sub drive_is_absolute_path {
+    my ($drive) = @_;
+    return PVE::Storage::classify_volume_id($drive->{file}, 1) eq $PVE::Storage::VOL_ABSOLUTE;
+}
+
 sub parse_drive_interface {
     my ($key) = @_;
 
-- 
2.47.3

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

[Max R. Carrara]

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/

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

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

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

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

[Max R. Carrara]

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/

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

[Fiona Ebner]

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.

Re: [PATCH qemu-server 2/2] use new classify_volume_id subroutine

[Fiona Ebner]

Am 28.07.26 um 2:22 PM schrieb Thomas Ellmenreich:
> @@ -766,6 +767,11 @@ sub drive_is_cdrom {
>      return $drive && $drive->{media} && ($drive->{media} eq 'cdrom');
>  }
>  
> +sub drive_is_absolute_path {

Nit: I'd prefer drive_has_absolute_path

> +    my ($drive) = @_;
> +    return PVE::Storage::classify_volume_id($drive->{file}, 1) eq $PVE::Storage::VOL_ABSOLUTE;
> +}
> +
>  sub parse_drive_interface {
>      my ($key) = @_;
>  

Re: [PATCH storage 1/2] add subroutine to classify volume ids

[Fiona Ebner]

Am 28.07.26 um 2:22 PM schrieb Thomas Ellmenreich:
> +sub classify_volume_id {

As already resulted from the other discussion, the current
implementation is rather for a classify_drive() helper. Technically, an
absolute path is not a valid volume ID. Our storage layer just sometimes
does not cleanly separate them.

> +    my ($volid, $noerr) = @_;
> +
> +    if (!defined($volid) || $volid eq "none") {

I think we should die for undef and make sure that we always do have an
explicit value when calling this function. It's not the same for the
pve-volume-id-or-qm-path format. The result from parse_drive() will
always have a value and otherwise something went wrong and most likely
points to another issue that should be resolved.

> +        return VOL_NONE;
> +    } elsif ($volid eq 'cdrom') {
> +        return VOL_CDROM;
> +    } elsif ($volid =~ m|^/|) {
> +        return VOL_ABSOLUTE;
> +    } elsif (parse_volume_id($volid, 1)) {
> +        return VOL_VOLUME;
> +    }
> +
> +    return undef if $noerr;
> +    die "unable to classify volume ID '$volid'";
> +}
> +
>  PVE::JSONSchema::register_format('pve-dir-override', \&verify_dir_override);
>  
>  sub verify_dir_override {

Re: [PATCH storage 1/2] add subroutine to classify volume ids

[Fiona Ebner]

Am 31.07.26 um 12:13 PM schrieb Fiona Ebner:
> Am 28.07.26 um 2:22 PM schrieb Thomas Ellmenreich:
>> +sub classify_volume_id {
> 
> As already resulted from the other discussion, the current
> implementation is rather for a classify_drive() helper. Technically, an

Or rather classify_drive_file() since it does not look at the whole drive.

> absolute path is not a valid volume ID. Our storage layer just sometimes
> does not cleanly separate them.
> 
>> +    my ($volid, $noerr) = @_;
>> +
>> +    if (!defined($volid) || $volid eq "none") {
> 
> I think we should die for undef and make sure that we always do have an
> explicit value when calling this function. It's not the same for the
> pve-volume-id-or-qm-path format. The result from parse_drive() will
> always have a value and otherwise something went wrong and most likely
> points to another issue that should be resolved.
> 
>> +        return VOL_NONE;
>> +    } elsif ($volid eq 'cdrom') {
>> +        return VOL_CDROM;
>> +    } elsif ($volid =~ m|^/|) {
>> +        return VOL_ABSOLUTE;
>> +    } elsif (parse_volume_id($volid, 1)) {
>> +        return VOL_VOLUME;
>> +    }
>> +
>> +    return undef if $noerr;
>> +    die "unable to classify volume ID '$volid'";
>> +}
>> +
>>  PVE::JSONSchema::register_format('pve-dir-override', \&verify_dir_override);
>>  
>>  sub verify_dir_override {
> 
> 
> 
> 
>