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 8E4121FF0E5 for ; Wed, 29 Jul 2026 10:28:00 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 285A020A75; Wed, 29 Jul 2026 10:28:00 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 29 Jul 2026 10:27:46 +0200 Message-Id: To: "Thomas Ellmenreich" , From: "Max R. Carrara" Subject: Re: [RFC qemu-server/storage 0/2] refactor of volume_id classification X-Mailer: aerc 0.18.2-0-ge037c095a049 References: <20260728122218.202963-1-t.ellmenreich@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1785313627545 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.027 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: KCFRQMDAB6VJAIVPSK4TUKWBPCGJYOGE X-Message-ID-Hash: KCFRQMDAB6VJAIVPSK4TUKWBPCGJYOGE 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 Tue Jul 28, 2026 at 4:29 PM CEST, Thomas Ellmenreich wrote: > 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 stil= l > > 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 / variabl= es > > 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. > > > - 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. > > > > > - Also, such functions should not throw an error at all -- you have > > noticed yourself that you always pass `$noerr`, so there's most likel= y > > 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 subroutine= s > > that each perform their own dedicated check, e.g.: > > > > use v5.36; > > > > use Exporter qw(import); > > > > our @EXPORT_OK =3D 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, thoug= h > > 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 fav= our > of the constants. Since those are not an option anymore, I don't find thi= s > approach that bad. > > > 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 wher= e > > 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 ::Helpe= rs. Oh yeah, that would of course be a good spot to place them! I hadn't checked for any existing places =F0=9F=98=87 > > > I don't really see a use case for it in pve-storage. Then, use a > > separate subroutine for each check. Once we see a need for these > > helpers elsewhere, we can start generalizing them, maybe even move > > them to pve-common or something similar. The existing subroutines in > > qemu-server could then just be wrappers for those in pve-common (or > > some other package). > > > > That way you can do your refactor in qemu-server and then later expan= d > > to other places, without having to worry about additional churn, or > > having to move constants around. > > > > I hope all of this makes sense -- I know it's a lot, but I wanted to > > share all of my thoughts here, esp. because I have stepped into many > > (way too many) little traps myself when it comes to refactoring Perl > > code. I do like the fact that you're tackling this a lot; I'm always a > > fan of reducing the number of random regexes and strings that are > > floating around. So, I hope my comments are of use to you! :) > > The comments are very much appreciated ^^ I'm very glad! If you have any more questions etc. please let me know, I'm always happy to help :) > > > > > [series] https://lore.proxmox.com/pve-devel/20260422111322.257380-1-m.c= arrara@proxmox.com/