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 64BEC1FF0E4 for ; Tue, 28 Jul 2026 15:30:56 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 23AC820968; Tue, 28 Jul 2026 15:30:56 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 28 Jul 2026 15:30:48 +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: <20260728122218.202963-1-t.ellmenreich@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1785245412101 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.028 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: YXMROU4ZKIF32LCJ4XXCMNTGOJIOXEKY X-Message-ID-Hash: YXMROU4ZKIF32LCJ4XXCMNTGOJIOXEKY 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 2:22 PM CEST, Thomas Ellmenreich wrote: > Refactor of volume_id classification > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > While developing this: [0] patch series, I refactored all the checks for > volume id's as absolute paths into their own subroutine. The original ser= ies > was then dropped in favour of a quicker fix: [1]. This series revives the > refactor without the fix and, unlike the original series creates a new > classify_volume_id subroutine, as @Fiona Ebener mentioned here: [2]. > > Overview > -------- > > As previously mentioned, a new classify_volume_id subroutine has been > introduced, which can then be used to determine the type of volume id. Th= is > type can be one of: > > > 'none', 'cdrom', 'absolute', 'volume' > > The subroutine and constants are exported by the pve-storage package and > used in the qemu-server package. > > Open Questions > -------------- > > - The classify subroutine currently throws an error if the volume id does= not > match any of the patterns (when 'noerr' is not selected). I'm not sure = if > this is the correct way to implement a classification method, given tha= t all > my uses enable the noerr option. That said, the case where an error is = thrown > should be very unlikely and would point to an error that should be fixe= d. > > - I have decided to only use the new function in the QemuServer.qm module= , > since other uses can then be added incrementally. Does this approach ma= ke > sense? > > [0]: https://lore.proxmox.com/pve-devel/20260722095251.89606-1-t.ellmenre= ich@proxmox.com/T/#t > [1]: https://lore.proxmox.com/pve-devel/20260724081611.11254-1-f.ebner@pr= oxmox.com/ > [2]: https://lore.proxmox.com/pve-devel/0b4272e1-3634-4fdd-b79f-6b077dd38= 7d5@proxmox.com/ > > [...] 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). - 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. 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. - 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. - 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. - 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. 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 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! :) [series] https://lore.proxmox.com/pve-devel/20260422111322.257380-1-m.carra= ra@proxmox.com/