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 09A341FF13A for ; Wed, 22 Jul 2026 16:01:56 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 499FC214D5; Wed, 22 Jul 2026 16:01:55 +0200 (CEST) Message-ID: <0b4272e1-3634-4fdd-b79f-6b077dd387d5@proxmox.com> Date: Wed, 22 Jul 2026 16:01:50 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH qemu-server v2 1/2] refactor checks for absolute path as drive To: Daniel Kral , Thomas Ellmenreich , pve-devel@lists.proxmox.com References: <20260722095251.89606-1-t.ellmenreich@proxmox.com> <20260722095251.89606-2-t.ellmenreich@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784728882478 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.224 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: 4ZRMJJ7K5KSA5JNI7AY57KVLSPVFPKI2 X-Message-ID-Hash: 4ZRMJJ7K5KSA5JNI7AY57KVLSPVFPKI2 X-MailFrom: f.ebner@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: Am 22.07.26 um 2:45 PM schrieb Daniel Kral: > On Wed Jul 22, 2026 at 11:52 AM CEST, Thomas Ellmenreich wrote: >> Previously there were a bunch of separate checks for whether a drive was >> an absolute path. These have been refactored to use two checks in >> QemuServer::Drive and QemuServer::Helpers. While these helpers are fine at a glance, a classify_drive_volid() similar to LXC/Config.pm's classify_mountpoint() might be a bit nicer in the long run. It could return 'absolute', 'none', 'cdrom' and 'volume'. >> @@ -913,7 +914,7 @@ PVE::JSONSchema::register_format( >> sub verify_volume_id_or_absolute_path { >> my ($volid, $noerr) = @_; >> >> - return $volid if $volid =~ m|^/|; >> + return $volid if PVE::QemuServer::Helpers::is_absolute_path($volid); >> >> $volid = eval { PVE::JSONSchema::check_format('pve-volume-id', $volid, '') }; >> if ($@) { >> @@ -1555,7 +1556,7 @@ sub print_vga_device { >> sub vm_is_volid_owner { >> my ($storecfg, $vmid, $volid) = @_; >> >> - if ($volid !~ m|^/|) { >> + if (!PVE::QemuServer::Helpers::is_absolute_path($volid)) { >> my ($path, $owner); >> eval { ($path, $owner) = PVE::Storage::path($storecfg, $volid); }; >> log_warn("ownership of volume '$volid' could not be determined: $@") if $@; >> @@ -1841,11 +1842,9 @@ sub destroy_vm { >> { include_unused => 1 }, >> sub { >> my ($ds, $drive) = @_; >> - return if drive_is_cdrom($drive); >> + return if drive_is_cdrom($drive) || drive_is_absolute_path($drive); >> >> my $volid = $drive->{file}; >> - return if !$volid || $volid =~ m|^/|; > > Hm, the !$volid check should be included in the check above as well as > drive_is_absolute_path() doesn't check this and shouldn't need to. > > Though if there's good reason that !$volid will never be true here, this > can be removed in a previous commit with a clear reasoning why. We should be protected. We don't allow empty values as part of a property string and even if, the schema validation against 'pve-volume-id-or-qm-path' would fail. The implementation in foreach_volume_full() in pve-guest-common won't call the closure if parsing fails. So I think dropping the check is fine. If it really triggers, it will just result in a warning pointing to a programming error somewhere else, which we should fix then.