From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 61AA51FF141 for ; Fri, 27 Feb 2026 12:49:19 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 26681F442; Fri, 27 Feb 2026 12:50:17 +0100 (CET) Message-ID: Date: Fri, 27 Feb 2026 12:49:42 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox-ve-rs 5/9] frr: add template serializer and serialize fabrics using templates To: pve-devel@lists.proxmox.com References: <20260203160246.353351-1-g.goller@proxmox.com> <20260203160246.353351-6-g.goller@proxmox.com> Content-Language: en-US From: Stefan Hanreich In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.343 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 1.158 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.306 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.668 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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: SFCKYWHHJAA3Z3OJ5NQFFGOQPRD5GINB X-Message-ID-Hash: SFCKYWHHJAA3Z3OJ5NQFFGOQPRD5GINB X-MailFrom: s.hanreich@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 2/25/26 5:29 PM, Gabriel Goller wrote: > On 25.02.2026 14:43, Stefan Hanreich wrote: >> A lot of structs here derive Builder, but we don't use any of the >> generated builder functionality atm and just instantiate the structs >> directly in ve-config since all the properties are pub. We should imo >> settle on one approach, preferably now, and then use that consistently. >> >> We use many collection/map types here, and with bon we'd have to write >> some boilerplate for those [1] and add validation for e.g. duplicate >> route_maps [2]. In the case of the BGP router this could become quite >> tricky imo, since for instance the value of ASN / remote-as / .. depends >> on whether we have a BGP controller / fabric / ... >> >> Additionally, we're creating an instance of FrrConfig in the Perl part, >> by just building the hash in a way that serializes / deserializes into >> the FrrConfig in Rust. With the builder pattern we'd have to expose the >> builder functionality to the perl side somehow. I think directly >> instantiating/mutating + (de)serializing is the better way here for that >> reason, since we get a partial FrrConfig from Perl already and then just >> incrementally add stuff in Rust. So it'd make sense to remove all >> Builder functionality from here for now to save on dependencies / >> compile-time? >> >> For future additions, e.g. route-maps, we could utilize the Builder >> pattern, since it is entirely contained to Rust and would make sense >> there - but I'd then introduce it in that patch series. >> >> [1] https://bon-rs.com/guide/typestate-api/builder-fields#custom-fields >> [2] https://bon-rs.com/guide/patterns/fallible-builders#fallible-builders > > Good point -- I'm definitely going through and removing the derive where it > isn't needed. > > As discussed offlist, this is not super-urgent as the api of proxmox-frr isn't > really stable or critical or something. Nevertheless we should check how much > the compilation-time overhead is for deriving bon for every struct, as bon is > not really lighweight (uses the type-state builder pattern). > > We also need to check if we want to add some validation here or if we don't need > that. If we need some validation, I'd go with bon, because it's a uniform method > of doing this, instead of mixing new() methods and TryInto,TryFrom > implementations, etc. > Quickly went over the build_fabrics code and didn't notice any struct that needs > "different" validations, so e.g. sometimes append, sometimes overwrite (so bon > would still work and we wouldn't need two creation methods). > > Don't know if we should clean this up and continue with bon, or rip it out > (which wouldn't be much work because we don't use it that heavy) and maybe add > this later on. > > Open for some other arguments or opinions! Had some further off-list discussion and settled on the current approach, without utilizing bon::Builder for now - mostly because it's hard to expose for the Perl side atm and we build part of the FRR configuration there still.