From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 314FA1FF0EA for ; Thu, 13 Aug 2026 12:50:11 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id B33AD21549; Thu, 13 Aug 2026 12:50:07 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 13 Aug 2026 12:50:02 +0200 Message-Id: To: "Thomas Ellmenreich" , Subject: Re: [PATCH qemu-server v3 2/3] add new classify_drive_file utility From: "Elias Huhsovitz" X-Mailer: aerc 0.20.0 References: <20260812121327.145140-1-t.ellmenreich@proxmox.com> <20260812121327.145140-3-t.ellmenreich@proxmox.com> In-Reply-To: <20260812121327.145140-3-t.ellmenreich@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1786618185869 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.801 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: ATXAVJOFOEFIJX333ENWDE5HTUVOD3QC X-Message-ID-Hash: ATXAVJOFOEFIJX333ENWDE5HTUVOD3QC X-MailFrom: e.huhsovitz@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Testing ------- I tested this using qm create with all affected drive types, e.g., qm create =20 --ide2 none,media=3Dcrom | cdrom | etc=20 --scsi0 local-lvm:5 | etc and then destroying them using qm destroy --purge It worked as intended. Small note ---------- Small note inline, but no change necessary IMHO. Therefore: Reviewed-by: Elias Huhsovitz Tested-by: Elias Huhsovitz 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 > --- > 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} =3D $volid; > } > =20 > - $drive->{media} =3D 'cdrom' if !$drive->{media} && $drive->{file} = =3D~ m/^(cdrom|none)$/; > + my $file_type =3D classify_drive_file($drive->{file}); > + > + $drive->{media} =3D 'cdrom' > + if !$drive->{media} && ($file_type eq "cdrom" || $file_type eq "= none"); > } > =20 > sub parse_hotplug_features { > @@ -1540,7 +1544,7 @@ sub print_vga_device { > sub vm_is_volid_owner { > my ($storecfg, $vmid, $volid) =3D @_; > =20 > - if ($volid !~ m|^/|) { > + if (classify_drive_file($volid) ne "absolute") { > my ($path, $owner); > eval { ($path, $owner) =3D 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); > =20 > my $volid =3D $drive->{file}; > - return if !$volid || $volid =3D~ m|^/|; > + return if !$volid || drive_has_absolute_path($drive); > =20 > my $result =3D eval { PVE::Storage::volume_is_base_and_u= sed($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); > =20 > my $volid =3D $drive->{file}; > - return if !$volid || $volid =3D~ m|^/|; > + return if !$volid || drive_has_absolute_path($drive); > return if $volids->{$volid}; > =20 > my ($path, $owner) =3D eval { PVE::Storage::path($storecfg, $vol= id) }; > @@ -3610,7 +3614,7 @@ sub config_to_command { > my $live_restore =3D $live_restore_backing->{$ds}; > =20 > 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 =3D > PVE::QemuServer::Blockdev::generate_throttle_gro= up($drive); > push @$cmd, '-object', to_json($throttle_group, { ca= nonical =3D> 1 }); > @@ -5289,7 +5293,10 @@ sub vmconfig_update_disk { > eval { PVE::QemuServer::Blockdev::change_medium($storecfg, $= vmid, $opt, $drive); }; > my $err =3D $@; > =20 > - if ($drive->{file} eq 'none' && drive_is_cloudinit($old_driv= e)) { > + if ( > + classify_drive_file($drive->{file}) eq "none" > + && drive_is_cloudinit($old_drive) > + ) { > vmconfig_register_unused_drive($storecfg, $vmid, $conf, = $old_drive); > } > =20 > @@ -6071,7 +6078,7 @@ sub get_vm_volumes { > sub { > my ($volid, $attr) =3D @_; > =20 > - return if $volid =3D~ m|^/|; > + return if classify_drive_file($volid) eq "absolute"; > =20 > my ($sid, $volname) =3D PVE::Storage::parse_volume_id($volid= , 1); > return if !$sid; > @@ -6095,7 +6102,7 @@ sub get_current_vm_volumes { > sub { > my ($ds, $drive) =3D @_; > =20 > - 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 =3D~ m/vzdump:([^\s:]*):(\S+)$/) { > my $volid =3D $2; > eval { > - if ($volid =3D~ 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 =3D sub { > return if drive_is_cdrom($drive, 1); > =20 > my $volid =3D $drive->{file}; > - return if !$volid || $volid =3D~ m|^/|; > + return if !$volid || drive_has_absolute_path($drive); > =20 > my ($path, $owner) =3D PVE::Storage::path($storecfg, $volid)= ; > return if !$path || !$owner || ($owner !=3D $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 =3D 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; > } > =20 > +# 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) =3D @_; > + > + return 'unknown' if !$volid; > + > + if ($volid eq 'none') { > + return 'none'; > + } > + > + if ($volid eq 'cdrom') { > + return 'cdrom'; > + } > + > + if ($volid =3D~ 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) =3D @_; > return $drive->{file} =3D~ m@[:/](?:vm-\d+-)?cloudinit(?:\.$QEMU_FOR= MAT_RE)?$@; > @@ -794,6 +820,11 @@ sub drive_is_cdrom { > return $drive && $drive->{media} && ($drive->{media} eq 'cdrom'); > } > =20 > +sub drive_has_absolute_path { > + my ($drive) =3D @_; > + return classify_drive_file($drive->{file}) eq "absolute"; > +} > + > sub parse_drive_interface { > my ($key) =3D @_; > =20