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 292FC1FF0ED for ; Fri, 31 Jul 2026 11:56:26 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 8CB2921436; Fri, 31 Jul 2026 11:56:25 +0200 (CEST) Message-ID: Date: Fri, 31 Jul 2026 11:56:21 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC qemu-server/storage 0/2] refactor of volume_id classification To: Thomas Ellmenreich , "Max R. Carrara" , pve-devel@lists.proxmox.com References: <20260728122218.202963-1-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: 1785491771985 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.156 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: IXO7VP7OFVRMPBCI5OAML6OJ5GEWDTOO X-Message-ID-Hash: IXO7VP7OFVRMPBCI5OAML6OJ5GEWDTOO 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 28.07.26 um 4:28 PM schrieb Thomas Ellmenreich: > First of all, thanks for taking a look ;) > > Some comments below. I will wait a little longer to see if anyone else > would like to leave some comments, but then I will send a v1. > > On Tue Jul 28, 2026 at 3:30 PM CEST, Max R. Carrara wrote: > > [snip] > >> Overall I like this idea a lot, but there are some things that IMO still >> need to be fleshed out more. A bunch of comments: >> >> - I'm personally not a fan of `use constant` anymore, since it's rather >> unwieldy and doesn't play too nice with the rest of Perl (e.g. >> interpolating those constants is annoying). > > good to know > >> >> - I would also refrain from exposing any additional constants / variables >> in PVE::Storage or PVE::Storage::Plugin, because those things >> implicitly become part of the public API of PVE::Storage, which in >> turn means it's really hard to change them later on. > > also good to know > >> In fact, in one of my recent [series] I'm actively trying to get rid >> of the `our $RE_.*` regexes in PVE::Storage for precisely that reason >> (see patches #22 - #28 there). >> >> Perl's lack of a type system also makes it rather annoying to model >> such things -- in Rust this would be just a neat enum and we could >> call it a day. > > Yes, that's what I was also thinking of, but perl does not make it easy. > I'm also not sure if we should go out of our way trying to model an actual type system in Perl. I think using strings in place of constants is fine in this case. >> - The other thing is, do we have a need for anything but 'volume' in >> pve-storage? Wouldn't it make sense for this helper to live somewhere >> else? >> >> If we decide to keep this in pve-storage, it should only remain in >> PVE::Storage or PVE::Storage::Common, and not PVE::Storage::Plugin, >> since ::Plugin should only contain things relevant for the storage >> plugin API. > > Yeah, I think you are right with your assumption. I was also considering > just leaving the subroutine in 'qemu_server', but then I found it fitting > to place it next to 'parse_volume_id'. That said, 'qemu_server' is the > better location, so I will place it there. Yes, I fully agree. It classifies a pve-volume-id-or-qm-path which the storage layer has no idea about ;) > >> >> - Also, such functions should not throw an error at all -- you have >> noticed yourself that you always pass `$noerr`, so there's most likely >> not a need for it to exist. If the caller throws an error directly >> if something doesn't match, it's easier to see what the intention >> behind the check is. > > :thumbs-up: > >> >> - So, with all that being said, I instead suggest using four subroutines >> that each perform their own dedicated check, e.g.: >> >> use v5.36; >> >> use Exporter qw(import); >> >> our @EXPORT_OK = qw( >> is_cdrom >> ); >> >> sub is_cdrom : prototype($) ($value) { >> return defined($value) && $value eq 'cdrom'; >> } >> >> ... and so on. >> >> Without checking in greater detail, these helper functions can >> probably live in their own module somewhere inside qemu-server, though >> I don't know where else you are planning to use these helpers. >> >> While it would probably be considered insane to use a separate >> function for each check in a normal programming language, Perl doesn't >> really provide any (sane) constructs (without 10.000 gotchas) that >> allow you to express this kind of thing in a neater manner. > > I was actually also considering this option, but then discarded it in favour > of the constants. Since those are not an option anymore, I don't find this > approach that bad. Not a fan of this idea. For one, we already have a drive_is_cdrom() and this would be way too easy to confuse with that. But it's also just unnecessary complexity. Just do it like classify_mountpoint(), it's simple, straightforward and does what it says. > >> Separate subroutines also make it easier to change the underlying >> implementation once necessary -- I doubt this will matter much here, >> since the checks are relatively simple, but if we e.g. use perlmod >> more and end up expressing this as an enum in Rust, then we could let >> those subroutines wrap the (single) function that we expose through >> perlmod instead. If we were to introduce new constants in Perl, this >> would not be possible -- you would have to modify each call site where >> those constants are used again. >> >> This is just an example, but I hope this makes sense. >> >> - To summarize all of my rambling above, I would suggest putting this >> into a dedicated module for such utils somewhere in qemu-server, as > > Should I really create a completely new module? I was thinking of placing them > in PVE::QemuServer::Helpers, but maybe that is not quite correct? And the > names would have to be a bit more explicit if they were placed in ::Helpers. I feel like it belongs into the QemuServer::Drive module. The verify_volume_id_or_qm_path and format registration for it could also move there.