* [PATCH qemu-server v3 0/3] refactor of volume_id classification
@ 2026-08-12 12:13 Thomas Ellmenreich
2026-08-12 12:13 ` [PATCH qemu-server v3 1/3] move verify_volume_id_or_* format registrations into Drive submodule Thomas Ellmenreich
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Thomas Ellmenreich @ 2026-08-12 12:13 UTC (permalink / raw)
To: pve-devel; +Cc: Thomas Ellmenreich
Refactor of volume_id classification
====================================
While developing this: [0] patch series, I refactored all the checks for
disk files 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_drive_file subroutine, as @Fiona Ebener mentioned here: [2].
Overview
--------
As previously mentioned, a new classify_drive_file subroutine has been
introduced, which can then be used to determine the type of disk file. This
type can be one of:
> 'none', 'cdrom', 'absolute', 'volume' and in the case of
> classification failure: 'unknown'
Other Small changes
-------------------
In the light of this small refactoring, 2 other small changes crept in.
(1) First, one to move the definitions and registration of 2 drive related
subroutines to the QemuServer::Drive submodule. Then (2), the removal of a
couple of undef checks for volids which are no longer necessary. These
changes have been placed in their own patches to be easily reverted.
Changes since v2 (thanks @Max, @Elias)
--------------------------------------
- Restructured the 'classify_drive_file' subroutine and added an early
return in the case of an undef valid so that all codepaths return a
valid string
- Changed description of the 'classify_drive_file' to name 'unknown'
instead of undef
- Improved the wording of different commit messages
Changes since v1 (thanks @Elias)
--------------------------------
- Instead of mixing double quotes (") and single quotes (') the
'classify_drive_file' function now only uses single quotes.
- Since returning undef for 'classify_drive_file' could lead to
unnessesary undef checking when comparing to expected values to avoid
warnings, the subroutine now returns 'unknown' in the case of a failed
classification.
Changes since RFC (thanks @Max, @Fiona)
---------------------------------------
- moved the classification subroutine out of PVE::Storage and into
PVE::QemuServer::Drive
- Rewrite of the subroutine with the following changes:
- does not throw errors anymore, only returns undef if
classification fails
- renamed to 'classify_drive_file'
- before, undef led to it being classified as 'none' which now
also just returns undef
- moved the registration of 'verify_volume_id_or_(absolute_path/qm_path)'
from QemuServer to QemuServer::Drive
- reintroduced the removal of some unnecessary undef checks as a
separate patch
[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/
qemu-manager:
Thomas Ellmenreich (3):
move verify_volume_id_or_* format registrations into Drive submodule
add new classify_drive_file utility
remove unnecessary undef checks
src/PVE/QemuServer.pm | 63 +++++++++++--------------------------
src/PVE/QemuServer/Drive.pm | 59 ++++++++++++++++++++++++++++++++++
2 files changed, 78 insertions(+), 44 deletions(-)
Summary over all repositories:
2 files changed, 78 insertions(+), 44 deletions(-)
--
Generated by murpp 0.12.0
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH qemu-server v3 1/3] move verify_volume_id_or_* format registrations into Drive submodule 2026-08-12 12:13 [PATCH qemu-server v3 0/3] refactor of volume_id classification Thomas Ellmenreich @ 2026-08-12 12:13 ` Thomas Ellmenreich 2026-08-12 12:13 ` [PATCH qemu-server v3 2/3] add new classify_drive_file utility Thomas Ellmenreich 2026-08-12 12:13 ` [PATCH qemu-server v3 3/3] remove unnecessary undef checks Thomas Ellmenreich 2 siblings, 0 replies; 7+ messages in thread From: Thomas Ellmenreich @ 2026-08-12 12:13 UTC (permalink / raw) To: pve-devel; +Cc: Thomas Ellmenreich, Elias Huhsovitz The format registrations of verify_volume_id_or_qm_path and verify_volume_id_or_absolute_path were moved from QemuServer to QemuServer::Drive, which is a more fitting location. Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com> Reviewed-by: Elias Huhsovitz <e.huhsovitz@proxmox.com> --- src/PVE/QemuServer.pm | 28 ---------------------------- src/PVE/QemuServer/Drive.pm | 28 ++++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm index c819c837..6c597e85 100644 --- a/src/PVE/QemuServer.pm +++ b/src/PVE/QemuServer.pm @@ -895,34 +895,6 @@ sub pve_verify_cpuset { return PVE::CpuSet->new($members)->short_string(); } -PVE::JSONSchema::register_format('pve-volume-id-or-qm-path', \&verify_volume_id_or_qm_path); - -sub verify_volume_id_or_qm_path { - my ($volid, $noerr) = @_; - - return $volid if $volid eq 'none' || $volid eq 'cdrom'; - - return verify_volume_id_or_absolute_path($volid, $noerr); -} - -PVE::JSONSchema::register_format( - 'pve-volume-id-or-absolute-path', - \&verify_volume_id_or_absolute_path, -); - -sub verify_volume_id_or_absolute_path { - my ($volid, $noerr) = @_; - - return $volid if $volid =~ m|^/|; - - $volid = eval { PVE::JSONSchema::check_format('pve-volume-id', $volid, '') }; - if ($@) { - return if $noerr; - die $@; - } - return $volid; -} - my $serialdesc = { optional => 1, type => 'string', diff --git a/src/PVE/QemuServer/Drive.pm b/src/PVE/QemuServer/Drive.pm index b80b7dbb..52e4a89b 100644 --- a/src/PVE/QemuServer/Drive.pm +++ b/src/PVE/QemuServer/Drive.pm @@ -753,6 +753,34 @@ sub verify_bootdisk { die "invalid boot disk '$value'\n"; } +PVE::JSONSchema::register_format('pve-volume-id-or-qm-path', \&verify_volume_id_or_qm_path); + +sub verify_volume_id_or_qm_path { + my ($volid, $noerr) = @_; + + return $volid if $volid eq 'none' || $volid eq 'cdrom'; + + return verify_volume_id_or_absolute_path($volid, $noerr); +} + +PVE::JSONSchema::register_format( + 'pve-volume-id-or-absolute-path', + \&verify_volume_id_or_absolute_path, +); + +sub verify_volume_id_or_absolute_path { + my ($volid, $noerr) = @_; + + return $volid if $volid =~ m|^/|; + + $volid = eval { PVE::JSONSchema::check_format('pve-volume-id', $volid, '') }; + if ($@) { + return if $noerr; + die $@; + } + return $volid; +} + sub drive_is_cloudinit { my ($drive) = @_; return $drive->{file} =~ m@[:/](?:vm-\d+-)?cloudinit(?:\.$QEMU_FORMAT_RE)?$@; -- 2.47.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH qemu-server v3 2/3] add new classify_drive_file utility 2026-08-12 12:13 [PATCH qemu-server v3 0/3] refactor of volume_id classification Thomas Ellmenreich 2026-08-12 12:13 ` [PATCH qemu-server v3 1/3] move verify_volume_id_or_* format registrations into Drive submodule Thomas Ellmenreich @ 2026-08-12 12:13 ` Thomas Ellmenreich 2026-08-13 10:50 ` Elias Huhsovitz 2026-08-12 12:13 ` [PATCH qemu-server v3 3/3] remove unnecessary undef checks Thomas Ellmenreich 2 siblings, 1 reply; 7+ messages in thread From: Thomas Ellmenreich @ 2026-08-12 12:13 UTC (permalink / raw) To: pve-devel; +Cc: Thomas Ellmenreich To more easily and clearly branch off different types of drive files a new utility function is introduced that classifies such a drive file. The return options are 'none', 'cdrom', 'absolute' and 'volume'. In any other case the function returns 'unknown'. Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com> --- src/PVE/QemuServer.pm | 29 ++++++++++++++++++----------- src/PVE/QemuServer/Drive.pm | 31 +++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 11 deletions(-) diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm index 6c597e85..d9d76ab3 100644 --- a/src/PVE/QemuServer.pm +++ b/src/PVE/QemuServer.pm @@ -43,7 +43,6 @@ use PVE::PBSClient; use PVE::RESTEnvironment qw(log_warn); use PVE::RPCEnvironment; use PVE::SafeSyslog; -use PVE::Storage; use PVE::SysFSTools; use PVE::Systemd; use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_foreach $IPV6RE); @@ -73,6 +72,8 @@ use PVE::QemuServer::CPUFlags; use PVE::QemuServer::Drive qw( is_valid_drivename checked_volume_format + classify_drive_file + drive_has_absolute_path drive_is_cloudinit drive_is_cdrom parse_drive @@ -1102,7 +1103,10 @@ sub cleanup_drive_path { $drive->{file} = $volid; } - $drive->{media} = 'cdrom' if !$drive->{media} && $drive->{file} =~ m/^(cdrom|none)$/; + my $file_type = classify_drive_file($drive->{file}); + + $drive->{media} = 'cdrom' + if !$drive->{media} && ($file_type eq "cdrom" || $file_type eq "none"); } sub parse_hotplug_features { @@ -1540,7 +1544,7 @@ sub print_vga_device { sub vm_is_volid_owner { my ($storecfg, $vmid, $volid) = @_; - if ($volid !~ m|^/|) { + if (classify_drive_file($volid) ne "absolute") { my ($path, $owner); eval { ($path, $owner) = PVE::Storage::path($storecfg, $volid); }; log_warn("ownership of volume '$volid' could not be determined: $@") if $@; @@ -1830,7 +1834,7 @@ sub destroy_vm { return if drive_is_cdrom($drive); my $volid = $drive->{file}; - return if !$volid || $volid =~ m|^/|; + return if !$volid || drive_has_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 @@ -1848,7 +1852,7 @@ sub destroy_vm { return if drive_is_cdrom($drive, 1); my $volid = $drive->{file}; - return if !$volid || $volid =~ m|^/|; + return if !$volid || drive_has_absolute_path($drive); return if $volids->{$volid}; my ($path, $owner) = eval { PVE::Storage::path($storecfg, $volid) }; @@ -3610,7 +3614,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 (classify_drive_file($drive->{file}) ne "none") { my $throttle_group = PVE::QemuServer::Blockdev::generate_throttle_group($drive); push @$cmd, '-object', to_json($throttle_group, { canonical => 1 }); @@ -5289,7 +5293,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 ( + classify_drive_file($drive->{file}) eq "none" + && drive_is_cloudinit($old_drive) + ) { vmconfig_register_unused_drive($storecfg, $vmid, $conf, $old_drive); } @@ -6071,7 +6078,7 @@ sub get_vm_volumes { sub { my ($volid, $attr) = @_; - return if $volid =~ m|^/|; + return if classify_drive_file($volid) eq "absolute"; my ($sid, $volname) = PVE::Storage::parse_volume_id($volid, 1); return if !$sid; @@ -6095,7 +6102,7 @@ sub get_current_vm_volumes { sub { my ($ds, $drive) = @_; - if (PVE::Storage::parse_volume_id($drive->{file}, 1)) { + if (classify_drive_file($drive->{file}) eq "volume") { check_volume_storage_type($storecfg, $drive->{file}); push $volumes->@*, $drive->{file}; } @@ -6471,7 +6478,7 @@ sub tar_restore_cleanup { if ($line =~ m/vzdump:([^\s:]*):(\S+)$/) { my $volid = $2; eval { - if ($volid =~ m|^/|) { + if (classify_drive_file($volid) eq "absolute") { unlink $volid || die 'unlink failed\n'; } else { PVE::Storage::vdisk_free($storecfg, $volid); @@ -6519,7 +6526,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_has_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 52e4a89b..efcfa2b1 100644 --- a/src/PVE/QemuServer/Drive.pm +++ b/src/PVE/QemuServer/Drive.pm @@ -21,6 +21,8 @@ our @EXPORT_OK = qw( is_valid_drivename checked_parse_volname checked_volume_format + classify_drive_file + drive_has_absolute_path drive_is_cloudinit drive_is_cdrom parse_drive @@ -781,6 +783,30 @@ sub verify_volume_id_or_absolute_path { return $volid; } +# Tries to classify the drive file as either 'none', 'cdrom', 'absolute' +# or 'volume'. In all other cases it will return 'unknown'. +sub classify_drive_file { + my ($volid) = @_; + + return 'unknown' if !$volid; + + if ($volid eq 'none') { + return 'none'; + } + + if ($volid eq 'cdrom') { + return 'cdrom'; + } + + if ($volid =~ m|^/|) { + return 'absolute'; + } + + return PVE::Storage::parse_volume_id($volid, 1) + ? 'volume' + : 'unknown'; +} + sub drive_is_cloudinit { my ($drive) = @_; return $drive->{file} =~ m@[:/](?:vm-\d+-)?cloudinit(?:\.$QEMU_FORMAT_RE)?$@; @@ -794,6 +820,11 @@ sub drive_is_cdrom { return $drive && $drive->{media} && ($drive->{media} eq 'cdrom'); } +sub drive_has_absolute_path { + my ($drive) = @_; + return classify_drive_file($drive->{file}) eq "absolute"; +} + sub parse_drive_interface { my ($key) = @_; -- 2.47.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH qemu-server v3 2/3] add new classify_drive_file utility 2026-08-12 12:13 ` [PATCH qemu-server v3 2/3] add new classify_drive_file utility Thomas Ellmenreich @ 2026-08-13 10:50 ` Elias Huhsovitz 2026-08-13 11:13 ` Thomas Ellmenreich 0 siblings, 1 reply; 7+ messages in thread From: Elias Huhsovitz @ 2026-08-13 10:50 UTC (permalink / raw) To: Thomas Ellmenreich, pve-devel Testing ------- I tested this using qm create with all affected drive types, e.g., qm create <vmid> --ide2 none,media=crom | cdrom | etc --scsi0 local-lvm:5 | etc and then destroying them using qm destroy <vmid> --purge It worked as intended. Small note ---------- Small note inline, but no change necessary IMHO. Therefore: Reviewed-by: Elias Huhsovitz <e.huhsovitz@proxmox.com> Tested-by: Elias Huhsovitz <e.huhsovitz@proxmox.com> On Wed Aug 12, 2026 at 2:13 PM CEST, Thomas Ellmenreich wrote: > To more easily and clearly branch off different types of drive files a > new utility function is introduced that classifies such a drive file. > The return options are 'none', 'cdrom', 'absolute' and 'volume'. In any > other case the function returns 'unknown'. > > Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com> > --- > src/PVE/QemuServer.pm | 29 ++++++++++++++++++----------- > src/PVE/QemuServer/Drive.pm | 31 +++++++++++++++++++++++++++++++ > 2 files changed, 49 insertions(+), 11 deletions(-) > > diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm > index 6c597e85..d9d76ab3 100644 > --- a/src/PVE/QemuServer.pm > +++ b/src/PVE/QemuServer.pm > @@ -43,7 +43,6 @@ use PVE::PBSClient; > use PVE::RESTEnvironment qw(log_warn); > use PVE::RPCEnvironment; > use PVE::SafeSyslog; > -use PVE::Storage; > use PVE::SysFSTools; > use PVE::Systemd; > use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_foreach $IPV6RE); > @@ -73,6 +72,8 @@ use PVE::QemuServer::CPUFlags; > use PVE::QemuServer::Drive qw( > is_valid_drivename > checked_volume_format > + classify_drive_file > + drive_has_absolute_path > drive_is_cloudinit > drive_is_cdrom > parse_drive > @@ -1102,7 +1103,10 @@ sub cleanup_drive_path { > $drive->{file} = $volid; > } > > - $drive->{media} = 'cdrom' if !$drive->{media} && $drive->{file} =~ m/^(cdrom|none)$/; > + my $file_type = classify_drive_file($drive->{file}); > + > + $drive->{media} = 'cdrom' > + if !$drive->{media} && ($file_type eq "cdrom" || $file_type eq "none"); > } > > sub parse_hotplug_features { > @@ -1540,7 +1544,7 @@ sub print_vga_device { > sub vm_is_volid_owner { > my ($storecfg, $vmid, $volid) = @_; > > - if ($volid !~ m|^/|) { > + if (classify_drive_file($volid) ne "absolute") { > my ($path, $owner); > eval { ($path, $owner) = PVE::Storage::path($storecfg, $volid); }; > log_warn("ownership of volume '$volid' could not be determined: $@") if $@; > @@ -1830,7 +1834,7 @@ sub destroy_vm { > return if drive_is_cdrom($drive); > > my $volid = $drive->{file}; > - return if !$volid || $volid =~ m|^/|; > + return if !$volid || drive_has_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 > @@ -1848,7 +1852,7 @@ sub destroy_vm { > return if drive_is_cdrom($drive, 1); > > my $volid = $drive->{file}; > - return if !$volid || $volid =~ m|^/|; > + return if !$volid || drive_has_absolute_path($drive); > return if $volids->{$volid}; > > my ($path, $owner) = eval { PVE::Storage::path($storecfg, $volid) }; > @@ -3610,7 +3614,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 (classify_drive_file($drive->{file}) ne "none") { > my $throttle_group = > PVE::QemuServer::Blockdev::generate_throttle_group($drive); > push @$cmd, '-object', to_json($throttle_group, { canonical => 1 }); > @@ -5289,7 +5293,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 ( > + classify_drive_file($drive->{file}) eq "none" > + && drive_is_cloudinit($old_drive) > + ) { > vmconfig_register_unused_drive($storecfg, $vmid, $conf, $old_drive); > } > > @@ -6071,7 +6078,7 @@ sub get_vm_volumes { > sub { > my ($volid, $attr) = @_; > > - return if $volid =~ m|^/|; > + return if classify_drive_file($volid) eq "absolute"; > > my ($sid, $volname) = PVE::Storage::parse_volume_id($volid, 1); > return if !$sid; > @@ -6095,7 +6102,7 @@ sub get_current_vm_volumes { > sub { > my ($ds, $drive) = @_; > > - if (PVE::Storage::parse_volume_id($drive->{file}, 1)) { > + if (classify_drive_file($drive->{file}) eq "volume") { > check_volume_storage_type($storecfg, $drive->{file}); > push $volumes->@*, $drive->{file}; > } > @@ -6471,7 +6478,7 @@ sub tar_restore_cleanup { > if ($line =~ m/vzdump:([^\s:]*):(\S+)$/) { > my $volid = $2; > eval { > - if ($volid =~ m|^/|) { > + if (classify_drive_file($volid) eq "absolute") { > unlink $volid || die 'unlink failed\n'; > } else { > PVE::Storage::vdisk_free($storecfg, $volid); > @@ -6519,7 +6526,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_has_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 52e4a89b..efcfa2b1 100644 > --- a/src/PVE/QemuServer/Drive.pm > +++ b/src/PVE/QemuServer/Drive.pm > @@ -21,6 +21,8 @@ our @EXPORT_OK = qw( > is_valid_drivename > checked_parse_volname > checked_volume_format > + classify_drive_file > + drive_has_absolute_path > drive_is_cloudinit > drive_is_cdrom > parse_drive > @@ -781,6 +783,30 @@ sub verify_volume_id_or_absolute_path { > return $volid; > } > > +# Tries to classify the drive file as either 'none', 'cdrom', 'absolute' > +# or 'volume'. In all other cases it will return 'unknown'. > +sub classify_drive_file { > + my ($volid) = @_; > + > + return 'unknown' if !$volid; > + > + if ($volid eq 'none') { > + return 'none'; > + } > + > + if ($volid eq 'cdrom') { > + return 'cdrom'; > + } > + > + if ($volid =~ m|^/|) { > + return 'absolute'; > + } > + > + return PVE::Storage::parse_volume_id($volid, 1) > + ? 'volume' > + : 'unknown'; > +} I think the previous if-else based approach was fine here, but this is also good. I just noticed one thing regarding this approach. Since we are no longer mimicking a switch-case statement we could combine the the 'none' and 'cdrom' cases into one like this: if ($volid eq 'none' || $volid eq 'cdrom') { return $volid; } But i think the current approach is good and you don't need to change this! > + > sub drive_is_cloudinit { > my ($drive) = @_; > return $drive->{file} =~ m@[:/](?:vm-\d+-)?cloudinit(?:\.$QEMU_FORMAT_RE)?$@; > @@ -794,6 +820,11 @@ sub drive_is_cdrom { > return $drive && $drive->{media} && ($drive->{media} eq 'cdrom'); > } > > +sub drive_has_absolute_path { > + my ($drive) = @_; > + return classify_drive_file($drive->{file}) eq "absolute"; > +} > + > sub parse_drive_interface { > my ($key) = @_; > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH qemu-server v3 2/3] add new classify_drive_file utility 2026-08-13 10:50 ` Elias Huhsovitz @ 2026-08-13 11:13 ` Thomas Ellmenreich 0 siblings, 0 replies; 7+ messages in thread From: Thomas Ellmenreich @ 2026-08-13 11:13 UTC (permalink / raw) To: Elias Huhsovitz, pve-devel Thank you for having tested this! On Thu Aug 13, 2026 at 12:50 PM CEST, Elias Huhsovitz wrote: > Testing > ------- > I tested this using qm create with all affected drive types, e.g., > qm create <vmid> > --ide2 none,media=crom | cdrom | etc > --scsi0 local-lvm:5 | etc > > and then destroying them using > qm destroy <vmid> --purge > > It worked as intended. > > Small note > ---------- > Small note inline, but no change necessary IMHO. > Therefore: > > Reviewed-by: Elias Huhsovitz <e.huhsovitz@proxmox.com> > Tested-by: Elias Huhsovitz <e.huhsovitz@proxmox.com> [snip] >> +# Tries to classify the drive file as either 'none', 'cdrom', 'absolute' >> +# or 'volume'. In all other cases it will return 'unknown'. >> +sub classify_drive_file { >> + my ($volid) = @_; >> + >> + return 'unknown' if !$volid; >> + >> + if ($volid eq 'none') { >> + return 'none'; >> + } >> + >> + if ($volid eq 'cdrom') { >> + return 'cdrom'; >> + } >> + >> + if ($volid =~ m|^/|) { >> + return 'absolute'; >> + } >> + >> + return PVE::Storage::parse_volume_id($volid, 1) >> + ? 'volume' >> + : 'unknown'; >> +} > > I think the previous if-else based approach was fine here, but this is > also good. I just noticed one thing regarding this approach. Since we > are no longer mimicking a switch-case statement we could combine the the > 'none' and 'cdrom' cases into one like this: I changed the if statements based on this comment [1], by @Max, which I do agree with. The ifelse chain is more packed and not as clear or modifiable as a chain of early returns. [1]: https://lore.proxmox.com/pve-devel/DKILUOJ6LFZ7.1X2WY1PSWHQYG@proxmox.com/ > if ($volid eq 'none' || $volid eq 'cdrom') { > return $volid; > } > > But i think the current approach is good and you don't need to change this! I actually also considered using this approach, but in the end decided against it, as I preferred having all of the possible return values written down instead of just returning the variable. (Which functionally is obviously the same thing). [snip] ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH qemu-server v3 3/3] remove unnecessary undef checks 2026-08-12 12:13 [PATCH qemu-server v3 0/3] refactor of volume_id classification Thomas Ellmenreich 2026-08-12 12:13 ` [PATCH qemu-server v3 1/3] move verify_volume_id_or_* format registrations into Drive submodule Thomas Ellmenreich 2026-08-12 12:13 ` [PATCH qemu-server v3 2/3] add new classify_drive_file utility Thomas Ellmenreich @ 2026-08-12 12:13 ` Thomas Ellmenreich 2026-08-13 11:21 ` Elias Huhsovitz 2 siblings, 1 reply; 7+ messages in thread From: Thomas Ellmenreich @ 2026-08-12 12:13 UTC (permalink / raw) To: pve-devel; +Cc: Thomas Ellmenreich The surrounding code guarantees that the value is always defined, making these checks unnecessary. Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com> --- src/PVE/QemuServer.pm | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm index d9d76ab3..0bedd334 100644 --- a/src/PVE/QemuServer.pm +++ b/src/PVE/QemuServer.pm @@ -1831,11 +1831,10 @@ sub destroy_vm { { include_unused => 1 }, sub { my ($ds, $drive) = @_; - return if drive_is_cdrom($drive); - my $volid = $drive->{file}; - return if !$volid || drive_has_absolute_path($drive); + return if drive_is_cdrom($drive) || drive_has_absolute_path($drive); + my $volid = $drive->{file}; 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 log_warn("failed to check if volume '$volid' is used by linked clones: $@") @@ -1852,8 +1851,7 @@ sub destroy_vm { return if drive_is_cdrom($drive, 1); my $volid = $drive->{file}; - return if !$volid || drive_has_absolute_path($drive); - return if $volids->{$volid}; + return if drive_has_absolute_path($drive) || $volids->{$volid}; my ($path, $owner) = eval { PVE::Storage::path($storecfg, $volid) }; log_warn("failed to get path and owner of volume '$volid': $@") if $@; @@ -6523,11 +6521,9 @@ my $restore_cleanup_oldconf = sub { sub { my ($ds, $drive) = @_; - return if drive_is_cdrom($drive, 1); + return if drive_is_cdrom($drive, 1) || drive_has_absolute_path($drive); my $volid = $drive->{file}; - return if !$volid || drive_has_absolute_path($drive); - my ($path, $owner) = PVE::Storage::path($storecfg, $volid); return if !$path || !$owner || ($owner != $vmid); -- 2.47.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH qemu-server v3 3/3] remove unnecessary undef checks 2026-08-12 12:13 ` [PATCH qemu-server v3 3/3] remove unnecessary undef checks Thomas Ellmenreich @ 2026-08-13 11:21 ` Elias Huhsovitz 0 siblings, 0 replies; 7+ messages in thread From: Elias Huhsovitz @ 2026-08-13 11:21 UTC (permalink / raw) To: Thomas Ellmenreich, pve-devel I tested this patch by in injecting die "ASSERT: undefined or empty drive file for '$ds'\n" if !defined($volid) || $volid eq ''; Then i created/destroyed/restored on the default 'local' and 'local-lvm' storages. Valid configurations never caused $volid to be undef. Reviewed-by: Elias Huhsovitz <e.huhsovitz@proxmox.com> Tested-by: Elias Huhsovitz <e.huhsovitz@proxmox.com> On Wed Aug 12, 2026 at 2:13 PM CEST, Thomas Ellmenreich wrote: > The surrounding code guarantees that the value is always defined, > making these checks unnecessary. > > Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com> > --- > src/PVE/QemuServer.pm | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm > index d9d76ab3..0bedd334 100644 > --- a/src/PVE/QemuServer.pm > +++ b/src/PVE/QemuServer.pm > @@ -1831,11 +1831,10 @@ sub destroy_vm { > { include_unused => 1 }, > sub { > my ($ds, $drive) = @_; > - return if drive_is_cdrom($drive); > > - my $volid = $drive->{file}; > - return if !$volid || drive_has_absolute_path($drive); > + return if drive_is_cdrom($drive) || drive_has_absolute_path($drive); > > + my $volid = $drive->{file}; > 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 > log_warn("failed to check if volume '$volid' is used by linked clones: $@") > @@ -1852,8 +1851,7 @@ sub destroy_vm { > return if drive_is_cdrom($drive, 1); > > my $volid = $drive->{file}; > - return if !$volid || drive_has_absolute_path($drive); > - return if $volids->{$volid}; > + return if drive_has_absolute_path($drive) || $volids->{$volid}; > > my ($path, $owner) = eval { PVE::Storage::path($storecfg, $volid) }; > log_warn("failed to get path and owner of volume '$volid': $@") if $@; > @@ -6523,11 +6521,9 @@ my $restore_cleanup_oldconf = sub { > sub { > my ($ds, $drive) = @_; > > - return if drive_is_cdrom($drive, 1); > + return if drive_is_cdrom($drive, 1) || drive_has_absolute_path($drive); > > my $volid = $drive->{file}; > - return if !$volid || drive_has_absolute_path($drive); > - > my ($path, $owner) = PVE::Storage::path($storecfg, $volid); > return if !$path || !$owner || ($owner != $vmid); > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-08-13 11:22 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-12 12:13 [PATCH qemu-server v3 0/3] refactor of volume_id classification Thomas Ellmenreich 2026-08-12 12:13 ` [PATCH qemu-server v3 1/3] move verify_volume_id_or_* format registrations into Drive submodule Thomas Ellmenreich 2026-08-12 12:13 ` [PATCH qemu-server v3 2/3] add new classify_drive_file utility Thomas Ellmenreich 2026-08-13 10:50 ` Elias Huhsovitz 2026-08-13 11:13 ` Thomas Ellmenreich 2026-08-12 12:13 ` [PATCH qemu-server v3 3/3] remove unnecessary undef checks Thomas Ellmenreich 2026-08-13 11:21 ` Elias Huhsovitz
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.