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 BE7551FF0E4 for ; Tue, 28 Jul 2026 16:29:15 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 8072221323; Tue, 28 Jul 2026 16:29:15 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 28 Jul 2026 16:29:10 +0200 Message-Id: Subject: Re: [RFC qemu-server/storage 0/2] refactor of volume_id classification From: "Thomas Ellmenreich" To: "Max R. Carrara" , X-Mailer: aerc 0.20.0 References: <20260728122218.202963-1-t.ellmenreich@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1785248914193 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.084 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: EXHK6FUQAEMSF3IQ54KRUYYAWR4WBUKI X-Message-ID-Hash: EXHK6FUQAEMSF3IQ54KRUYYAWR4WBUKI X-MailFrom: t.ellmenreich@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: 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. > - 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 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 =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, 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 favou= r of the constants. Since those are not an option anymore, I don't find this 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 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 t= hem 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 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 expand > 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 ^^ > > [series] https://lore.proxmox.com/pve-devel/20260422111322.257380-1-m.car= rara@proxmox.com/