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 E18F21FF17A for ; Tue, 9 Dec 2025 13:26:32 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id BBDBF275F9; Tue, 9 Dec 2025 13:27:11 +0100 (CET) Mime-Version: 1.0 Date: Tue, 09 Dec 2025 13:26:36 +0100 Message-Id: From: "Christoph Heiss" To: "Lukas Wagner" X-Mailer: aerc 0.21.0 References: <20251205112528.373387-1-c.heiss@proxmox.com> <20251205112528.373387-3-c.heiss@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1765283191095 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.053 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [self.parts, proxmox.com] Subject: Re: [pdm-devel] [PATCH proxmox v2 02/14] network-types: move `Fqdn` type from proxmox-installer-common X-BeenThere: pdm-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Datacenter Manager development discussion Cc: Proxmox Datacenter Manager development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pdm-devel-bounces@lists.proxmox.com Sender: "pdm-devel" Thanks for the review! On Tue Dec 9, 2025 at 10:13 AM CET, Lukas Wagner wrote: > Looks good in general, I assume this was reviewed before this it was > moved from somewhere else. > > One note inline. > > Reviewed-by: Lukas Wagner > > On Fri Dec 5, 2025 at 12:25 PM CET, Christoph Heiss wrote: [..] >> +impl Fqdn { >> + /// Maximum length of a single label of the FQDN >> + const MAX_LABEL_LENGTH: usize = 63; >> + /// Maximum total length of the FQDN >> + const MAX_LENGTH: usize = 253; >> + >> + pub fn from(fqdn: &str) -> Result { >> + if fqdn.len() > Self::MAX_LENGTH { >> + return Err(FqdnParseError::TooLong(fqdn.len())); >> + } >> + >> + let parts = fqdn >> + .split('.') >> + .map(ToOwned::to_owned) >> + .collect::>(); >> + >> + for part in &parts { >> + if !Self::validate_single(part) { >> + return Err(FqdnParseError::InvalidPart(part.clone())); >> + } >> + } >> + >> + if parts.len() < 2 { >> + Err(FqdnParseError::MissingHostname) > > (a) > > Since `Fqdn::from` seems to be the only way to instantiate the Fqdn > type, and since the parts.len() < 2 establishes the invariant that the > number of parts can only ever be >= 2, > >> + } else if parts[0].chars().all(|c| c.is_ascii_digit()) { >> + // Do not allow a purely numeric hostname, see: >> + // https://bugzilla.proxmox.com/show_bug.cgi?id=1054 >> + Err(FqdnParseError::NumericHostname) >> + } else { >> + Ok(Self { parts }) >> + } >> + } >> + >> + pub fn host(&self) -> Option<&str> { >> + self.has_host().then_some(&self.parts[0]) >> + } > > (c) > > ... therefore you could assume that it is safe to just access > self.parts[0] and avoid the `Option<..>` in the return type? > > I'm always on board with being careful, but since this type is > relatively small, I think it would be reasonable to rely on the > invariant, which makes the call sites a bit nicer since it does not have > to handle the Option. > > What do you think? That seems very sensible, thanks for reasoning about this! The given invariant is also already established by the current unit tests below, so fine from my side too. Less `Option`s to handle at callsites is always a nice improvement IMO. I will change it for v2, it's a good chance to improve upon things. > >> + >> + pub fn domain(&self) -> String { >> + let parts = if self.has_host() { >> + &self.parts[1..] >> + } else { >> + &self.parts >> + }; >> + >> + parts.join(".") >> + } >> + >> + /// Checks whether the FQDN has a hostname associated with it, i.e. is has more than 1 part. >> + fn has_host(&self) -> bool { >> + self.parts.len() > 1 >> + } > > (b) > > ... which means that this should always return true, ... > _______________________________________________ pdm-devel mailing list pdm-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel