From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id BE4541FF0E3 for ; Tue, 04 Aug 2026 16:24:04 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id EE80021710; Tue, 04 Aug 2026 16:24:03 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 04 Aug 2026 16:23:58 +0200 Message-Id: Subject: Re: [PATCH qemu-server 2/3] add new classify_drive_file utility From: "Elias Huhsovitz" To: "Thomas Ellmenreich" , X-Mailer: aerc 0.20.0 References: <20260803121300.138287-1-t.ellmenreich@proxmox.com> <20260803121300.138287-3-t.ellmenreich@proxmox.com> In-Reply-To: <20260803121300.138287-3-t.ellmenreich@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1785853426893 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.107 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_LOW -0.7 Sender listed at https://www.dnswl.org/, low 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: NOZBXQUFNTIOZ5IDJWWP3H25UBRPGEJK X-Message-ID-Hash: NOZBXQUFNTIOZ5IDJWWP3H25UBRPGEJK 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: Good idea to unify the checking for drive type! Summary ------- * no explicit default return path * undef return depends on perl version & previous code. * inconsistent string quotes * small bug near your changes Comments inline On Mon Aug 3, 2026 at 2:12 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 undef. > Signed-off-by: Thomas Ellmenreich > --- > src/PVE/QemuServer.pm | 29 ++++++++++++++++++----------- > src/PVE/QemuServer/Drive.pm | 23 +++++++++++++++++++++++ > 2 files changed, 41 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; > } > =20 > - $drive->{media} =3D 'cdrom' if !$drive->{media} && $drive->{file} = =3D~ m/^(cdrom|none)$/; > + my $file_type =3D classify_drive_file($drive->{file}); ^^^^^ This variable is not ready to be undef. (See comments on classify_drive_fi= le below) > + > + $drive->{media} =3D 'cdrom' > + if !$drive->{media} && ($file_type eq "cdrom" || $file_type eq "= none"); > } > =20 > sub parse_hotplug_features { > @@ -1527,7 +1531,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 $@; > @@ -1817,7 +1821,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 > @@ -1835,7 +1839,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) }; > @@ -3597,7 +3601,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 }); > @@ -5276,7 +5280,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 > @@ -6058,7 +6065,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; > @@ -6082,7 +6089,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}; > } > @@ -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'; Should be "unlink failed\n" instead of 'unlink failed'. Otherwise we render the literal '\' + 'n'. I know this isn't your change, but i noticed this ;D. > } else { > PVE::Storage::vdisk_free($storecfg, $volid); > @@ -6506,7 +6513,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..f2106141 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,22 @@ 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 undef. > +sub classify_drive_file { > + my ($volid) =3D @_; > + > + if ($volid eq "none") { > + return "none"; > + } elsif ($volid eq 'cdrom') { Different string quotes for 'cdrom'. Other strings have double quotes. e.g., "none", "cdrom". I would suggest to only use single quotes here, since we do not have any string interpolation anyways. > + return "cdrom"; > + } elsif ($volid =3D~ m|^/|) { > + return "absolute"; > + } elsif (PVE::Storage::parse_volume_id($volid, 1)) { > + return "volume"; > + } > +} Missing default return path. The comments claim that the default path is undef, but this is not necessarily true and depends on the perl version and or the last executed statement. Even if the default path is always undef, this might lead to issues down the line. Some callers of this function are not prepared to handle undef (see comment futher up in the `cleanup_drive_path` subroutine). I would sug= gest returning a string that singals an error, e.g., an empty string '' > + > sub drive_is_cloudinit { > my ($drive) =3D @_; > return $drive->{file} =3D~ m@[:/](?:vm-\d+-)?cloudinit(?:\.$QEMU_FOR= MAT_RE)?$@; > @@ -794,6 +812,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