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 00C1B1FF138 for ; Wed, 18 Feb 2026 17:40:36 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E2F133E3; Wed, 18 Feb 2026 17:41:34 +0100 (CET) Message-ID: Date: Wed, 18 Feb 2026 17:41:28 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH pve-cluster 03/14 v2] pmxcfs-rs: add pmxcfs-config crate To: Kefu Chai , pve-devel@lists.proxmox.com References: <20260213094119.2379288-1-k.chai@proxmox.com> <20260213094119.2379288-4-k.chai@proxmox.com> Content-Language: en-US From: Samuel Rufinatscha In-Reply-To: <20260213094119.2379288-4-k.chai@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.245 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: 2J2LQ6CMSOU3B7TDZGPOYB2HGN52T2BX X-Message-ID-Hash: 2J2LQ6CMSOU3B7TDZGPOYB2HGN52T2BX X-MailFrom: s.rufinatscha@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: Thanks for the patch, Kefu! Some comments inline. On 2/13/26 10:42 AM, Kefu Chai wrote: > Add configuration management crate for pmxcfs: > - Config struct: Runtime configuration (node name, IP, flags) > - Thread-safe debug level mutation via RwLock Small issue here, uses AtomicU8 with the latest changes > - Arc-wrapped for shared ownership across components > - Comprehensive unit tests including thread safety tests > > This crate provides the foundational configuration structure used > by all pmxcfs components. The Config is designed to be shared via > Arc to allow multiple components to access the same configuration > instance, with mutable debug level for runtime adjustments. > > Signed-off-by: Kefu Chai > --- > src/pmxcfs-rs/Cargo.toml | 5 + > src/pmxcfs-rs/pmxcfs-config/Cargo.toml | 19 + > src/pmxcfs-rs/pmxcfs-config/README.md | 15 + > src/pmxcfs-rs/pmxcfs-config/src/lib.rs | 521 +++++++++++++++++++++++++ > 4 files changed, 560 insertions(+) > create mode 100644 src/pmxcfs-rs/pmxcfs-config/Cargo.toml > create mode 100644 src/pmxcfs-rs/pmxcfs-config/README.md > create mode 100644 src/pmxcfs-rs/pmxcfs-config/src/lib.rs > > diff --git a/src/pmxcfs-rs/Cargo.toml b/src/pmxcfs-rs/Cargo.toml > index 13407f402..f190968ed 100644 > --- a/src/pmxcfs-rs/Cargo.toml > +++ b/src/pmxcfs-rs/Cargo.toml > @@ -2,6 +2,7 @@ > [workspace] > members = [ > "pmxcfs-api-types", # Shared types and error definitions > + "pmxcfs-config", # Configuration management > ] > resolver = "2" > > @@ -16,10 +17,14 @@ rust-version = "1.85" > [workspace.dependencies] > # Internal workspace dependencies > pmxcfs-api-types = { path = "pmxcfs-api-types" } > +pmxcfs-config = { path = "pmxcfs-config" } > > # Error handling > thiserror = "1.0" The tracing dependency needs to be added in the workspace config > > +# Concurrency primitives > +parking_lot = "0.12" This is not needed anymore ... > + > # System integration > libc = "0.2" > > diff --git a/src/pmxcfs-rs/pmxcfs-config/Cargo.toml b/src/pmxcfs-rs/pmxcfs-config/Cargo.toml > new file mode 100644 > index 000000000..a1aeba1d3 > --- /dev/null > +++ b/src/pmxcfs-rs/pmxcfs-config/Cargo.toml > @@ -0,0 +1,19 @@ > +[package] > +name = "pmxcfs-config" > +description = "Configuration management for pmxcfs" > + > +version.workspace = true > +edition.workspace = true > +authors.workspace = true > +license.workspace = true > +repository.workspace = true > + > +[lints] > +workspace = true > + > +[dependencies] > +# Concurrency primitives > +parking_lot.workspace = true .. as this is unused > + > +# Logging > +tracing.workspace = true > diff --git a/src/pmxcfs-rs/pmxcfs-config/README.md b/src/pmxcfs-rs/pmxcfs-config/README.md > new file mode 100644 > index 000000000..53aaf443a > --- /dev/null > +++ b/src/pmxcfs-rs/pmxcfs-config/README.md > @@ -0,0 +1,15 @@ > +# pmxcfs-config > + > +**Configuration Management** for pmxcfs. > + > +This crate provides configuration structures for the pmxcfs daemon. > + > +## Overview > + > +The `Config` struct holds daemon-wide configuration including: > +- Node hostname > +- IP address > +- www-data group ID > +- Debug flag > +- Local mode flag > +- Cluster name > diff --git a/src/pmxcfs-rs/pmxcfs-config/src/lib.rs b/src/pmxcfs-rs/pmxcfs-config/src/lib.rs > new file mode 100644 > index 000000000..dca3c76b1 > --- /dev/null > +++ b/src/pmxcfs-rs/pmxcfs-config/src/lib.rs > @@ -0,0 +1,521 @@ > +use std::net::IpAddr; > +use std::sync::atomic::{AtomicU8, Ordering}; > +use std::sync::Arc; > + > +/// Global configuration for pmxcfs > +pub struct Config { > + /// Node name (hostname without domain) The validation code below allows dots, please re-visit > + nodename: String, > + > + /// Node IP address > + node_ip: IpAddr, > + > + /// www-data group ID for file permissions > + www_data_gid: u32, > + > + /// Force local mode (no clustering) > + local_mode: bool, > + > + /// Cluster name (CPG group name) > + cluster_name: String, > + > + /// Debug level (0 = normal, 1+ = debug) - mutable at runtime > + debug_level: AtomicU8, > +} > + > +impl Clone for Config { > + fn clone(&self) -> Self { > + Self { > + nodename: self.nodename.clone(), > + node_ip: self.node_ip, > + www_data_gid: self.www_data_gid, > + local_mode: self.local_mode, > + cluster_name: self.cluster_name.clone(), > + debug_level: AtomicU8::new(self.debug_level.load(Ordering::Relaxed)), > + } > + } > +} Do we need this Clone impl actually? If not we could remove it to avoid confusion with Arc::clone() > + > +impl std::fmt::Debug for Config { > + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { > + f.debug_struct("Config") > + .field("nodename", &self.nodename) > + .field("node_ip", &self.node_ip) > + .field("www_data_gid", &self.www_data_gid) > + .field("local_mode", &self.local_mode) > + .field("cluster_name", &self.cluster_name) > + .field("debug_level", &self.debug_level.load(Ordering::Relaxed)) > + .finish() > + } > +} > + > +impl Config { > + /// Validate a hostname according to RFC 1123 > + /// > + /// Hostname requirements: > + /// - Length: 1-253 characters > + /// - Labels (dot-separated parts): 1-63 characters each > + /// - Characters: alphanumeric and hyphens > + /// - Cannot start or end with hyphen > + /// - Case insensitive (lowercase preferred) > + fn validate_hostname(hostname: &str) -> Result<(), String> { > + if hostname.is_empty() { > + return Err("Hostname cannot be empty".to_string()); > + } > + if hostname.len() > 253 { > + return Err(format!("Hostname too long: {} > 253 characters", hostname.len())); > + } > + > + for label in hostname.split('.') { > + if label.is_empty() { > + return Err("Hostname cannot have empty labels (consecutive dots)".to_string()); > + } > + if label.len() > 63 { > + return Err(format!("Hostname label '{}' too long: {} > 63 characters", label, label.len())); > + } > + if label.starts_with('-') || label.ends_with('-') { > + return Err(format!("Hostname label '{}' cannot start or end with hyphen", label)); > + } > + if !label.chars().all(|c| c.is_ascii_alphanumeric() || c == '-') { > + return Err(format!("Hostname label '{}' contains invalid characters (only alphanumeric and hyphen allowed)", label)); > + } > + } > + > + Ok(()) > + } > + > + pub fn new( > + nodename: String, Into / &str could be nicer here. Also for the other String field below. > + node_ip: IpAddr, > + www_data_gid: u32, > + debug: bool, Maybe we should od also debug_level: u8 here? There is a setter below with also expects debug_level: u8 If we align this, we could avoid the bool to u8 conversion/indirection. > + local_mode: bool, > + cluster_name: String, > + ) -> Self { > + // Validate hostname (log warning but don't fail - matches C behavior) > + // The C implementation accepts any hostname from uname() without validation The first comment says "log warning but don't fail - matches C behavior" but the second says C does no validation at all. Please clarify :) If C does not validate, does not log about validity, and does not fail we maybe shouldnt do it on the Rust side too (for behavioral consistency), what do you think? > + if let Err(e) = Self::validate_hostname(&nodename) { > + tracing::warn!("Invalid nodename '{}': {}", nodename, e); nit: eventually use structured fields if we decide to log tracing::warn!(nodename = %nodename, error = %e, "invalid nodename"); > + } > + > + let debug_level = if debug { 1 } else { 0 }; > + Self { > + nodename, > + node_ip, > + www_data_gid, > + local_mode, > + cluster_name, > + debug_level: AtomicU8::new(debug_level), > + } > + } > + > + pub fn shared( > + nodename: String, > + node_ip: IpAddr, > + www_data_gid: u32, > + debug: bool, > + local_mode: bool, > + cluster_name: String, > + ) -> Arc { > + Arc::new(Self::new(nodename, node_ip, www_data_gid, debug, local_mode, cluster_name)) > + } nit: maybe we should even change this to the following to avoid duplication of all parameters of new()? pub fn into_shared(self) -> Arc { Arc::new(self) } so we only need to maintain one signature on future changes > + > + pub fn cluster_name(&self) -> &str { > + &self.cluster_name > + } > + > + pub fn nodename(&self) -> &str { > + &self.nodename > + } > + > + pub fn node_ip(&self) -> IpAddr { > + self.node_ip > + } > + > + pub fn www_data_gid(&self) -> u32 { > + self.www_data_gid > + } > + > + pub fn is_debug(&self) -> bool { > + self.debug_level() > 0 > + } > + > + pub fn is_local_mode(&self) -> bool { > + self.local_mode > + } > + > + /// Get current debug level (0 = normal, 1+ = debug) > + pub fn debug_level(&self) -> u8 { > + self.debug_level.load(Ordering::Relaxed) > + } > + > + /// Set debug level (0 = normal, 1+ = debug) > + pub fn set_debug_level(&self, level: u8) { > + self.debug_level.store(level, Ordering::Relaxed); > + } > +} > + > +#[cfg(test)] > +mod tests { > + //! Unit tests for Config struct > + //! > + //! This test module provides comprehensive coverage for: > + //! - Configuration creation and initialization > + //! - Getter methods for all configuration fields > + //! - Debug level mutation and thread safety > + //! - Concurrent access patterns (reads and writes) > + //! - Clone independence > + //! - Debug formatting > + //! - Edge cases (empty strings, long strings, special characters, unicode) > + //! > + //! ## Thread Safety > + //! > + //! The Config struct uses `AtomicU8` for debug_level to allow > + //! safe concurrent reads and writes. Tests verify: > + //! - 10 threads × 100 operations (concurrent modifications) > + //! - 20 threads × 1000 operations (concurrent reads) > + //! > + //! ## Edge Cases > + //! > + //! Tests cover various edge cases including: > + //! - Empty strings for node/cluster names > + //! - Long strings (1000+ characters) > + //! - Special characters in strings > + //! - Unicode support (emoji, non-ASCII characters) > + > + use super::*; > + use std::thread; > + > + // ===== Basic Construction Tests ===== > + > + #[test] > + fn test_config_creation() { > + let config = Config::new( > + "node1".to_string(), > + "192.168.1.10".parse().unwrap(), > + 33, > + false, > + false, > + "pmxcfs".to_string(), > + ); > + > + assert_eq!(config.nodename(), "node1"); > + assert_eq!(config.node_ip(), "192.168.1.10".parse::().unwrap()); > + assert_eq!(config.www_data_gid(), 33); > + assert!(!config.is_debug()); > + assert!(!config.is_local_mode()); > + assert_eq!(config.cluster_name(), "pmxcfs"); > + assert_eq!( > + config.debug_level(), > + 0, > + "Debug level should be 0 when debug is false" > + ); > + } > + > + #[test] > + fn test_config_creation_with_debug() { > + let config = Config::new( > + "node2".to_string(), > + "10.0.0.5".parse().unwrap(), > + 1000, > + true, > + false, > + "test-cluster".to_string(), > + ); > + > + assert!(config.is_debug()); > + assert_eq!( > + config.debug_level(), > + 1, > + "Debug level should be 1 when debug is true" > + ); > + } > + > + #[test] > + fn test_config_creation_local_mode() { > + let config = Config::new( > + "localhost".to_string(), > + "127.0.0.1".parse().unwrap(), > + 33, > + false, > + true, > + "local".to_string(), > + ); > + > + assert!(config.is_local_mode()); > + assert!(!config.is_debug()); > + } > + > + // ===== Getter Tests ===== > + > + #[test] > + fn test_all_getters() { > + let config = Config::new( > + "testnode".to_string(), > + "172.16.0.1".parse().unwrap(), > + 999, > + true, > + true, > + "my-cluster".to_string(), > + ); > + > + // Test all getter methods > + assert_eq!(config.nodename(), "testnode"); > + assert_eq!(config.node_ip(), "172.16.0.1".parse::().unwrap()); > + assert_eq!(config.www_data_gid(), 999); > + assert!(config.is_debug()); > + assert!(config.is_local_mode()); > + assert_eq!(config.cluster_name(), "my-cluster"); > + assert_eq!(config.debug_level(), 1); > + } > + > + // ===== Debug Level Mutation Tests ===== > + > + #[test] > + fn test_debug_level_mutation() { > + let config = Config::new( > + "node1".to_string(), > + "192.168.1.1".parse().unwrap(), > + 33, > + false, > + false, > + "pmxcfs".to_string(), > + ); > + > + assert_eq!(config.debug_level(), 0); > + > + config.set_debug_level(1); > + assert_eq!(config.debug_level(), 1); > + > + config.set_debug_level(5); > + assert_eq!(config.debug_level(), 5); > + > + config.set_debug_level(0); > + assert_eq!(config.debug_level(), 0); > + } > + > + #[test] > + fn test_debug_level_max_value() { > + let config = Config::new( > + "node1".to_string(), > + "192.168.1.1".parse().unwrap(), > + 33, > + false, > + false, > + "pmxcfs".to_string(), > + ); > + > + config.set_debug_level(255); > + assert_eq!(config.debug_level(), 255); > + > + config.set_debug_level(0); > + assert_eq!(config.debug_level(), 0); > + } > + > + // ===== Thread Safety Tests ===== > + > + #[test] > + fn test_debug_level_thread_safety() { > + let config = Config::shared( > + "node1".to_string(), > + "192.168.1.1".parse().unwrap(), > + 33, > + false, > + false, > + "pmxcfs".to_string(), > + ); > + > + let config_clone = Arc::clone(&config); > + > + // Spawn multiple threads that concurrently modify debug level > + let handles: Vec<_> = (0..10) > + .map(|i| { > + let cfg = Arc::clone(&config); > + thread::spawn(move || { > + for _ in 0..100 { > + cfg.set_debug_level(i); > + let _ = cfg.debug_level(); > + } > + }) > + }) > + .collect(); > + > + // All threads should complete without panicking > + for handle in handles { > + handle.join().unwrap(); > + } > + > + // Final value should be one of the values set by threads > + let final_level = config_clone.debug_level(); > + assert!( > + final_level < 10, > + "Debug level should be < 10, got {final_level}" > + ); > + } > + > + #[test] > + fn test_concurrent_reads() { > + let config = Config::shared( > + "node1".to_string(), > + "192.168.1.1".parse().unwrap(), > + 33, > + true, > + false, > + "pmxcfs".to_string(), > + ); > + > + // Spawn multiple threads that concurrently read config > + let handles: Vec<_> = (0..20) > + .map(|_| { > + let cfg = Arc::clone(&config); > + thread::spawn(move || { > + for _ in 0..1000 { > + assert_eq!(cfg.nodename(), "node1"); > + assert_eq!(cfg.node_ip(), "192.168.1.1".parse::().unwrap()); > + assert_eq!(cfg.www_data_gid(), 33); > + assert!(cfg.is_debug()); > + assert!(!cfg.is_local_mode()); > + assert_eq!(cfg.cluster_name(), "pmxcfs"); > + } > + }) > + }) > + .collect(); > + > + for handle in handles { > + handle.join().unwrap(); > + } > + } > + > + // ===== Clone Tests ===== > + > + #[test] > + fn test_config_clone() { > + let config1 = Config::new( > + "node1".to_string(), > + "192.168.1.1".parse().unwrap(), > + 33, > + true, > + false, > + "pmxcfs".to_string(), > + ); > + > + config1.set_debug_level(5); > + > + let config2 = config1.clone(); > + > + // Cloned config should have same values > + assert_eq!(config2.nodename(), config1.nodename()); > + assert_eq!(config2.node_ip(), config1.node_ip()); > + assert_eq!(config2.www_data_gid(), config1.www_data_gid()); > + assert_eq!(config2.is_debug(), config1.is_debug()); > + assert_eq!(config2.is_local_mode(), config1.is_local_mode()); > + assert_eq!(config2.cluster_name(), config1.cluster_name()); > + assert_eq!(config2.debug_level(), 5); > + > + // Modifying one should not affect the other > + config2.set_debug_level(10); > + assert_eq!(config1.debug_level(), 5); > + assert_eq!(config2.debug_level(), 10); > + } > + > + // ===== Debug Formatting Tests ===== > + > + #[test] > + fn test_debug_format() { > + let config = Config::new( > + "node1".to_string(), > + "192.168.1.1".parse().unwrap(), > + 33, > + true, > + false, > + "pmxcfs".to_string(), > + ); > + > + let debug_str = format!("{config:?}"); > + > + // Check that debug output contains all fields > + assert!(debug_str.contains("Config")); > + assert!(debug_str.contains("nodename")); > + assert!(debug_str.contains("node1")); > + assert!(debug_str.contains("node_ip")); > + assert!(debug_str.contains("192.168.1.1")); > + assert!(debug_str.contains("www_data_gid")); > + assert!(debug_str.contains("33")); > + assert!(debug_str.contains("local_mode")); > + assert!(debug_str.contains("false")); > + assert!(debug_str.contains("cluster_name")); > + assert!(debug_str.contains("pmxcfs")); > + assert!(debug_str.contains("debug_level")); > + } > + > + // ===== Edge Cases and Boundary Tests ===== > + > + #[test] > + fn test_empty_strings() { > + let config = Config::new( > + String::new(), > + "127.0.0.1".parse().unwrap(), > + 0, > + false, > + false, > + String::new(), > + ); > + > + assert_eq!(config.nodename(), ""); > + assert_eq!(config.node_ip(), "127.0.0.1".parse::().unwrap()); > + assert_eq!(config.cluster_name(), ""); > + assert_eq!(config.www_data_gid(), 0); > + } > + > + #[test] > + fn test_long_strings() { > + let long_name = "a".repeat(1000); > + let long_cluster = "cluster-".to_string() + &"x".repeat(500); > + > + let config = Config::new( > + long_name.clone(), > + "192.168.1.1".parse().unwrap(), > + u32::MAX, > + true, > + true, > + long_cluster.clone(), > + ); > + > + assert_eq!(config.nodename(), long_name); > + assert_eq!(config.node_ip(), "192.168.1.1".parse::().unwrap()); > + assert_eq!(config.cluster_name(), long_cluster); > + assert_eq!(config.www_data_gid(), u32::MAX); > + } > + > + #[test] > + fn test_special_characters_in_strings() { > + let config = Config::new( > + "node-1_test.local".to_string(), > + "192.168.1.10".parse().unwrap(), > + 33, > + false, > + false, > + "my-cluster_v2.0".to_string(), > + ); > + > + assert_eq!(config.nodename(), "node-1_test.local"); > + assert_eq!(config.node_ip(), "192.168.1.10".parse::().unwrap()); > + assert_eq!(config.cluster_name(), "my-cluster_v2.0"); > + } > + > + #[test] > + fn test_unicode_in_strings() { > + let config = Config::new( > + "ノード1".to_string(), > + "::1".parse().unwrap(), > + 33, > + false, > + false, > + "集群".to_string(), > + ); > + > + assert_eq!(config.nodename(), "ノード1"); > + assert_eq!(config.node_ip(), "::1".parse::().unwrap()); > + assert_eq!(config.cluster_name(), "集群"); > + } If we keep the validate_hostname() we should also have relevant tests for it > +}