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 1ED971FF0E6 for ; Fri, 07 Aug 2026 11:30:51 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 8D8B9214BC; Fri, 07 Aug 2026 11:30:50 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Fri, 07 Aug 2026 11:30:45 +0200 Message-Id: To: "Thomas Ellmenreich" , From: "Max R. Carrara" Subject: Re: [PATCH qemu-server v2 2/3] add new classify_drive_file utility X-Mailer: aerc 0.18.2-0-ge037c095a049 References: <20260805082122.43184-1-t.ellmenreich@proxmox.com> <20260805082122.43184-3-t.ellmenreich@proxmox.com> In-Reply-To: <20260805082122.43184-3-t.ellmenreich@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1786095029820 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.792 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: 2UMFWPBIXN7MUSRS3SDSM6LCUYNR6KKZ X-Message-ID-Hash: 2UMFWPBIXN7MUSRS3SDSM6LCUYNR6KKZ X-MailFrom: m.carrara@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: On Wed Aug 5, 2026 at 10:21 AM 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 | 25 +++++++++++++++++++++++++ > 2 files changed, 43 insertions(+), 11 deletions(-) > > diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm > index fdef20dc..0e998cbe 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; > } > > - $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"); > } > > sub parse_hotplug_features { > @@ -1527,7 +1531,7 @@ sub print_vga_device { > sub vm_is_volid_owner { > my ($storecfg, $vmid, $volid) =3D @_; > > - 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 $@; > @@ -1817,7 +1821,7 @@ sub destroy_vm { > return if drive_is_cdrom($drive); > > my $volid =3D $drive->{file}; > - return if !$volid || $volid =3D~ m|^/|; > + return if !$volid || drive_has_absolute_path($drive); > > 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 > @@ -1835,7 +1839,7 @@ sub destroy_vm { > return if drive_is_cdrom($drive, 1); > > my $volid =3D $drive->{file}; > - return if !$volid || $volid =3D~ m|^/|; > + return if !$volid || drive_has_absolute_path($drive); > return if $volids->{$volid}; > > my ($path, $owner) =3D eval { PVE::Storage::path($storecfg, $vol= id) }; > @@ -3597,7 +3601,7 @@ sub config_to_command { > my $live_restore =3D $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 =3D > PVE::QemuServer::Blockdev::generate_throttle_gro= up($drive); > push @$cmd, '-object', to_json($throttle_group, { ca= nonical =3D> 1 }); > @@ -5276,7 +5280,10 @@ sub vmconfig_update_disk { > eval { PVE::QemuServer::Blockdev::change_medium($storecfg, $= vmid, $opt, $drive); }; > my $err =3D $@; > > - 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); > } > > @@ -6058,7 +6065,7 @@ sub get_vm_volumes { > sub { > my ($volid, $attr) =3D @_; > > - return if $volid =3D~ m|^/|; > + return if classify_drive_file($volid) eq "absolute"; > > my ($sid, $volname) =3D PVE::Storage::parse_volume_id($volid= , 1); > return if !$sid; > @@ -6082,7 +6089,7 @@ sub get_current_vm_volumes { > sub { > my ($ds, $drive) =3D @_; > > - 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}; > } > @@ -6458,7 +6465,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); > @@ -6506,7 +6513,7 @@ my $restore_cleanup_oldconf =3D sub { > return if drive_is_cdrom($drive, 1); > > my $volid =3D $drive->{file}; > - return if !$volid || $volid =3D~ m|^/|; > + return if !$volid || drive_has_absolute_path($drive); > > 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..5a9b5ae2 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,24 @@ 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 undef. > +sub classify_drive_file { > + my ($volid) =3D @_; > + > + if ($volid eq 'none') { > + return 'none'; > + } elsif ($volid eq 'cdrom') { > + return 'cdrom'; > + } elsif ($volid =3D~ m|^/|) { > + return 'absolute'; > + } elsif (PVE::Storage::parse_volume_id($volid, 1)) { > + return 'volume'; > + } > + > + return 'unknown'; > +} Small note regarding style: While the above is completely fine (and you shouldn't change it!) I do wanna note that for more complex subroutines, I would personally prefer avoiding if-elsif chains, mainly because.. 1. they tend to become a little too "packed" when it comes to reading the code 2. they *may* inadvertently make future changes harder if a little more complex logic is involved To give you an example, have a look at some of our older code in `pve-storage` [0] that I'm trying to refactor at the moment. Eventually I replace each branch one-by-one in my series [1], until I finally condense the refactored logic in (yet) another patch [2]. Again, the code above is completely fine! Please don't feel like you have to refresh your series. I just wanted to mention it for future cases where the chains might be longer and the conditions are nastier :P [0]: https://git.proxmox.com/?p=3Dpve-storage.git;a=3Dblob;f=3Dsrc/PVE/Stor= age/Plugin.pm;h=3D4f69f9b5db69674335eb3024d61d4a3430bca1ec;hb=3Drefs/heads/= master#l799 [1]: https://lore.proxmox.com/pve-devel/20260422111322.257380-23-m.carrara@= proxmox.com/#Z31src:PVE:Storage:Plugin.pm [2]: https://lore.proxmox.com/pve-devel/20260422111322.257380-30-m.carrara@= proxmox.com/#Z31src:PVE:Storage:Plugin.pm > + > sub drive_is_cloudinit { > my ($drive) =3D @_; > return $drive->{file} =3D~ m@[:/](?:vm-\d+-)?cloudinit(?:\.$QEMU_FOR= MAT_RE)?$@; > @@ -794,6 +814,11 @@ sub drive_is_cdrom { > return $drive && $drive->{media} && ($drive->{media} eq 'cdrom'); > } > > +sub drive_has_absolute_path { > + my ($drive) =3D @_; > + return classify_drive_file($drive->{file}) eq "absolute"; > +} > + > sub parse_drive_interface { > my ($key) =3D @_; >