From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 921D31FF13F for ; Thu, 26 Mar 2026 14:57:20 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 22C2D1424C; Thu, 26 Mar 2026 14:57:41 +0100 (CET) Message-ID: <638c7623-b608-4328-ae93-cb561c58596d@proxmox.com> Date: Thu, 26 Mar 2026 14:57:07 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox-perl-rs 1/3] pve-rs: sdn: add route maps module To: Wolfgang Bumiller References: <20260325094142.174364-1-s.hanreich@proxmox.com> <20260325094142.174364-13-s.hanreich@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.712 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 0.001 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.001 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.001 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: 7DUCWNEB3RR7IWUCKXZSLAIVQNSVCA5B X-Message-ID-Hash: 7DUCWNEB3RR7IWUCKXZSLAIVQNSVCA5B 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 CC: pve-devel@lists.proxmox.com 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 3/26/26 11:31 AM, Wolfgang Bumiller wrote: [snip] >> + >> + /// Returns a list of all RouteMap entries. > > ^ Method: fixed this, and all other occurences, in the prefix list module as well >> + #[export] >> + pub fn list( >> + #[try_from_ref] this: &PerlRouteMapConfig, >> + ) -> Result, Error> { >> + Ok(this >> + .route_maps >> + .lock() >> + .unwrap() >> + .iter() >> + .map(|(id, route_map_entry)| { >> + let ConfigRouteMap::RouteMapEntry(route_map) = route_map_entry; >> + (id.clone(), route_map.clone().into()) >> + }) >> + .collect()) >> + } >> + >> + /// Returns a list of all RouteMap entries for a given RouteMap ID. > > ^ Method: > > and it doesn't return a list - which in perl is quite specific. > (Note that perlmod gained support for returning a *list* via > `perlmod::ser::Return`'s `List(T)` variant, as well as a `@`-like final > list parameter via the `#[list]` attribute. Also note that the latter is > not meant to be used for anything other than implementing a pre-existing > *perl* API) yeah that makes sense as well. this only refers to the comment though, not the method name afaict? might make sense to potentially rename the method as well? fixed the comment for now, in the prefix list module as well >> + #[export] >> + pub fn create( >> + #[try_from_ref] this: &PerlRouteMapConfig, >> + route_map: ApiRouteMap, >> + ) -> Result<(), Error> { >> + let mut route_maps = this.route_maps.lock().unwrap(); >> + >> + let id = >> + RouteMapEntryId::new(route_map.route_map_id().clone(), route_map.order()).to_string(); > > So the key we use in `route_maps` is constructed from the route_map's id > and order... > >> + let config_route_map = ConfigRouteMap::RouteMapEntry(route_map.into()); >> + >> + if route_maps.get(&id).is_some() { >> + anyhow::bail!("route map entry already exists in configuration: {}", id); >> + } >> + >> + route_maps.insert(id, config_route_map); > > ^ The above two should probably use the entry api > > route_maps.entry() { > Entry::Occupied(_) => bail!(...), > Entry::Vacant(vacancy) => vacancy.insert(...), > } fixed here and in the prefix list module! >> + >> + Ok(()) >> + } >> + >> + /// Returns a specfic entry of a RouteMap. >> + #[export] >> + pub fn get( >> + #[try_from_ref] this: &PerlRouteMapConfig, >> + route_map_id: RouteMapId, >> + order: u32, >> + ) -> Result, Error> { >> + let id = RouteMapEntryId::new(route_map_id, order); >> + >> + Ok(this >> + .route_maps >> + .lock() >> + .unwrap() >> + .iter() >> + .find(|(_id, route_map_entry)| { >> + let ConfigRouteMap::RouteMapEntry(route_map) = route_map_entry; >> + route_map.id() == &id > > ...so could we just `.get()` with a `RouteMapEntryId::new(route_map_id, > order).to_string()` here? yeah, idk what happened there - fixed it here and in the prefix list module! [snip]