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 CFA9F1FF138 for ; Wed, 18 Feb 2026 16:05:33 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2A7F21F8AD; Wed, 18 Feb 2026 16:06:31 +0100 (CET) Message-ID: <038df46b-770e-4f64-8905-ccb60367935e@proxmox.com> Date: Wed, 18 Feb 2026 16:06:24 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH pve-cluster 02/14 v2] pmxcfs-rs: add pmxcfs-api-types crate To: Kefu Chai , pve-devel@lists.proxmox.com References: <20260213094119.2379288-1-k.chai@proxmox.com> <20260213094119.2379288-3-k.chai@proxmox.com> Content-Language: en-US From: Samuel Rufinatscha In-Reply-To: <20260213094119.2379288-3-k.chai@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.246 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: ONGQA6A6RUH2ONMW3HQ76VV2BGKU3X5Y X-Message-ID-Hash: ONGQA6A6RUH2ONMW3HQ76VV2BGKU3X5Y 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. Looking across the series, only PmxcfsError::System and PmxcfsError::Configuration are actually constructed. The remaining variants, to_errno() and the defined Result seem unused. For my own understanding, how and when are they actually used? Also the README mentions automatic errno conversion. What does automatic mean here, and when is that triggered? I couldnt find a to_errno() usage. If not needed, please trim the unused variants and to_errno() to just what the series actually needs. On 2/13/26 10:41 AM, Kefu Chai wrote: > Add pmxcfs-api-types crate which provides foundational types: > - PmxcfsError: Error type with errno mapping for FUSE operations > - FuseMessage: Filesystem operation messages > - KvStoreMessage: Status synchronization messages > - ApplicationMessage: Wrapper enum for both message types > - VmType: VM type enum (Qemu, Lxc) FuseMessage, KvStoreMessage and ApplicationMessage are not present in this diff. Consider a more high-level prose message. IMO the message doesnt necessarly need to mention the actual PmxcfsError or VmType type names. > > All other crates will depend on these shared type definitions. > > Signed-off-by: Kefu Chai > --- > src/pmxcfs-rs/Cargo.toml | 10 +- > src/pmxcfs-rs/pmxcfs-api-types/Cargo.toml | 19 +++ > src/pmxcfs-rs/pmxcfs-api-types/README.md | 88 ++++++++++++++ > src/pmxcfs-rs/pmxcfs-api-types/src/error.rs | 122 ++++++++++++++++++++ > src/pmxcfs-rs/pmxcfs-api-types/src/lib.rs | 67 +++++++++++ > 5 files changed, 305 insertions(+), 1 deletion(-) > create mode 100644 src/pmxcfs-rs/pmxcfs-api-types/Cargo.toml > create mode 100644 src/pmxcfs-rs/pmxcfs-api-types/README.md > create mode 100644 src/pmxcfs-rs/pmxcfs-api-types/src/error.rs > create mode 100644 src/pmxcfs-rs/pmxcfs-api-types/src/lib.rs > > diff --git a/src/pmxcfs-rs/Cargo.toml b/src/pmxcfs-rs/Cargo.toml > index d109221fb..13407f402 100644 > --- a/src/pmxcfs-rs/Cargo.toml > +++ b/src/pmxcfs-rs/Cargo.toml > @@ -1,6 +1,7 @@ > # Workspace root for pmxcfs Rust implementation > [workspace] > members = [ > + "pmxcfs-api-types", # Shared types and error definitions > ] > resolver = "2" > > @@ -13,7 +14,14 @@ repository = "https://git.proxmox.com/?p=pve-cluster.git" > rust-version = "1.85" > > [workspace.dependencies] > -# Dependencies will be added incrementally as crates are introduced > +# Internal workspace dependencies > +pmxcfs-api-types = { path = "pmxcfs-api-types" } > + > +# Error handling > +thiserror = "1.0" > + > +# System integration > +libc = "0.2" > > [workspace.lints.clippy] > uninlined_format_args = "warn" > diff --git a/src/pmxcfs-rs/pmxcfs-api-types/Cargo.toml b/src/pmxcfs-rs/pmxcfs-api-types/Cargo.toml > new file mode 100644 > index 000000000..cdce7951a > --- /dev/null > +++ b/src/pmxcfs-rs/pmxcfs-api-types/Cargo.toml > @@ -0,0 +1,19 @@ > +[package] > +name = "pmxcfs-api-types" > +description = "Shared types and error definitions for pmxcfs" > + > +version.workspace = true > +edition.workspace = true > +authors.workspace = true > +license.workspace = true > +repository.workspace = true > + > +[lints] > +workspace = true > + > +[dependencies] > +# Error handling > +thiserror.workspace = true > + > +# System integration > +libc.workspace = true > diff --git a/src/pmxcfs-rs/pmxcfs-api-types/README.md b/src/pmxcfs-rs/pmxcfs-api-types/README.md > new file mode 100644 > index 000000000..ddcd4e478 > --- /dev/null > +++ b/src/pmxcfs-rs/pmxcfs-api-types/README.md The README needs a revisit. Type names and API surface are better documented through rustdoc on the code itself, the README should focus on why this crate exists and what a new developer needs to understand before looking at the code. Please also check the other patches, if this can be improved. The errno mapping table doesn't need to be in the README, I think. To keep it brief, I think this can be looked up in the code. The "Error Handling" section is not that informative and a bit hard to follow. On the Rust side it only mentions Result without further explaination how this maps? I assume the "automatic errno conversion"? If this is important, please note this. Also it mentions cfs-utils.h for error codes, but I couldnt find error codes there. The "Known Issues / TODOs" section can be dropped if anyways "None identified". I would keep it more brief. Something a long the following lines would likely be enough for this crate (and if the to_errno is still required, please also include it) # pmxcfs-api-types This crate provides shared types and error definitions used across all pmxcfs crates. Having them in a dedicated crate with no internal dependencies avoids circular dependencies between the higher-level crates. Note that OpenVZ (historically present in the C implementation) is not represented, it was dropped in PVE 4.0. ## References - [xyz](../actual link to C file) - ... > @@ -0,0 +1,88 @@ > +# pmxcfs-api-types > + > +**Shared Types and Error Definitions** for pmxcfs. > + > +This crate provides common types and error definitions used across all pmxcfs crates. > + > +## Overview > + > +The crate contains: > +- **Error types**: `PmxcfsError` with errno mapping for FUSE > +- **Shared types**: `MemberInfo`, `NodeSyncInfo`, `VmType`, `VmEntry` > + > +## Error Types > + > +### PmxcfsError > + > +Type-safe error enum with automatic errno conversion. > + > +### errno Mapping > + > +Errors automatically convert to POSIX errno values for FUSE. > + > +| Error | errno | Value | Note | > +|-------|-------|-------|------| > +| `NotFound(_)` | `ENOENT` | 2 | File or directory not found | > +| `PermissionDenied` | `EACCES` | 13 | File permission denied | > +| `AlreadyExists(_)` | `EEXIST` | 17 | File already exists | > +| `NotADirectory(_)` | `ENOTDIR` | 20 | Not a directory | > +| `IsADirectory(_)` | `EISDIR` | 21 | Is a directory | > +| `DirectoryNotEmpty(_)` | `ENOTEMPTY` | 39 | Directory not empty | > +| `InvalidArgument(_)` | `EINVAL` | 22 | Invalid argument | > +| `InvalidPath(_)` | `EINVAL` | 22 | Invalid path | > +| `FileTooLarge` | `EFBIG` | 27 | File too large | > +| `ReadOnlyFilesystem` | `EROFS` | 30 | Read-only filesystem | > +| `NoQuorum` | `EACCES` | 13 | No cluster quorum | > +| `Lock(_)` | `EAGAIN` | 11 | Lock unavailable, try again | > +| `Timeout` | `ETIMEDOUT` | 110 | Operation timed out | > +| `Io(e)` | varies | varies | OS error code or `EIO` | > +| Others* | `EIO` | 5 | Internal error | > + > +*Others include: `Database`, `Fuse`, `Cluster`, `Corosync`, `Configuration`, `System`, `Ipc` > + > +## Shared Types > + > +### MemberInfo > + > +Cluster member information. > + > +### NodeSyncInfo > + > +DFSM synchronization state. > + > +### VmType > + > +VM/CT type enum (Qemu or Lxc). > + > +### VmEntry > + > +VM/CT entry for vmlist. > + > +## C to Rust Mapping > + > +### Error Handling > + > +**C Version (cfs-utils.h):** > +- Return codes: `0` = success, negative = error > +- errno-based error reporting > +- Manual error checking everywhere > + > +**Rust Version:** > +- `Result` type > + > +## Known Issues / TODOs > + > +### Missing Features > +- None identified > + > +### Compatibility > +- **errno values**: Match POSIX standards > + > +## References > + > +### C Implementation > +- `src/pmxcfs/cfs-utils.h` - Utility types and error codes This file does not contain error codes, maybe wrong ref? > + > +### Related Crates > +- **pmxcfs-dfsm**: Uses shared types for cluster sync > +- **pmxcfs-memdb**: Uses PmxcfsError for database operations I could not find any PmxcfsError usage in these two crates. Please re-visit. > diff --git a/src/pmxcfs-rs/pmxcfs-api-types/src/error.rs b/src/pmxcfs-rs/pmxcfs-api-types/src/error.rs > new file mode 100644 > index 000000000..dcb5d1e9e > --- /dev/null > +++ b/src/pmxcfs-rs/pmxcfs-api-types/src/error.rs > @@ -0,0 +1,122 @@ > +use thiserror::Error; > + > +/// Error types for pmxcfs operations > +#[derive(Error, Debug)] > +pub enum PmxcfsError { > + #[error("I/O error: {0}")] > + Io(#[from] std::io::Error), > + > + #[error("Database error: {0}")] > + Database(String), > + > + #[error("FUSE error: {0}")] > + Fuse(String), > + > + #[error("Cluster error: {0}")] > + Cluster(String), > + > + #[error("Corosync error: {0}")] > + Corosync(String), > + > + #[error("Configuration error: {0}")] > + Configuration(String), > + > + #[error("System error: {0}")] > + System(String), > + > + #[error("IPC error: {0}")] > + Ipc(String), > + > + #[error("Permission denied")] > + PermissionDenied, > + > + #[error("Not found: {0}")] > + NotFound(String), > + > + #[error("Already exists: {0}")] > + AlreadyExists(String), > + > + #[error("Invalid argument: {0}")] > + InvalidArgument(String), > + > + #[error("Not a directory: {0}")] > + NotADirectory(String), > + > + #[error("Is a directory: {0}")] > + IsADirectory(String), > + > + #[error("Directory not empty: {0}")] > + DirectoryNotEmpty(String), > + > + #[error("No quorum")] > + NoQuorum, > + > + #[error("Read-only filesystem")] > + ReadOnlyFilesystem, > + > + #[error("File too large")] > + FileTooLarge, > + > + #[error("Filesystem full")] > + FilesystemFull, > + > + #[error("Lock error: {0}")] > + Lock(String), > + > + #[error("Timeout")] > + Timeout, > + > + #[error("Invalid path: {0}")] > + InvalidPath(String), > +} > + > +impl PmxcfsError { > + /// Convert error to errno value for FUSE operations > + pub fn to_errno(&self) -> i32 { > + match self { > + // File/directory errors > + PmxcfsError::NotFound(_) => libc::ENOENT, > + PmxcfsError::AlreadyExists(_) => libc::EEXIST, > + PmxcfsError::NotADirectory(_) => libc::ENOTDIR, > + PmxcfsError::IsADirectory(_) => libc::EISDIR, > + PmxcfsError::DirectoryNotEmpty(_) => libc::ENOTEMPTY, > + PmxcfsError::FileTooLarge => libc::EFBIG, > + PmxcfsError::FilesystemFull => libc::ENOSPC, > + PmxcfsError::ReadOnlyFilesystem => libc::EROFS, > + > + // Permission and access errors > + // EACCES: Permission denied for file operations (standard POSIX) > + // C implementation uses EACCES as default for access/quorum issues > + PmxcfsError::PermissionDenied => libc::EACCES, > + PmxcfsError::NoQuorum => libc::EACCES, > + > + // Validation errors > + PmxcfsError::InvalidArgument(_) => libc::EINVAL, > + PmxcfsError::InvalidPath(_) => libc::EINVAL, > + > + // Lock errors - use EAGAIN for temporary failures > + PmxcfsError::Lock(_) => libc::EAGAIN, > + > + // Timeout > + PmxcfsError::Timeout => libc::ETIMEDOUT, > + > + // I/O errors with automatic errno extraction > + PmxcfsError::Io(e) => match e.raw_os_error() { > + Some(errno) => errno, > + None => libc::EIO, > + }, > + > + // Fallback to EIO for internal/system errors > + PmxcfsError::Database(_) | > + PmxcfsError::Fuse(_) | > + PmxcfsError::Cluster(_) | > + PmxcfsError::Corosync(_) | > + PmxcfsError::Configuration(_) | > + PmxcfsError::System(_) | > + PmxcfsError::Ipc(_) => libc::EIO, > + } > + } > +} > + > +/// Result type for pmxcfs operations > +pub type Result = std::result::Result; > diff --git a/src/pmxcfs-rs/pmxcfs-api-types/src/lib.rs b/src/pmxcfs-rs/pmxcfs-api-types/src/lib.rs > new file mode 100644 > index 000000000..99cafbaa3 > --- /dev/null > +++ b/src/pmxcfs-rs/pmxcfs-api-types/src/lib.rs > @@ -0,0 +1,67 @@ > +mod error; > + > +pub use error::{PmxcfsError, Result}; > + > +/// Maximum size for status data (matches C implementation) > +/// From status.h: #define CFS_MAX_STATUS_SIZE (32 * 1024) > +pub const CFS_MAX_STATUS_SIZE: usize = 32 * 1024; This const is only used in the status crate. Do we need to share it here? > + > +/// VM/CT types > +/// > +/// Note: OpenVZ was historically supported (VMTYPE_OPENVZ = 2 in C implementation) > +/// but was removed in PVE 4.0 in favor of LXC. Only QEMU and LXC are currently supported. Thanks for adding this note! > +#[derive(Debug, Clone, Copy, PartialEq, Eq)] > +pub enum VmType { > + Qemu, > + Lxc, > +} > + > +impl VmType { > + /// Returns the directory name where config files are stored > + pub fn config_dir(&self) -> &'static str { > + match self { > + VmType::Qemu => "qemu-server", > + VmType::Lxc => "lxc", > + } > + } > +} > + > +impl std::fmt::Display for VmType { > + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { > + match self { > + VmType::Qemu => write!(f, "qemu"), > + VmType::Lxc => write!(f, "lxc"), > + } > + } > +} > + > +/// VM/CT entry for vmlist > +#[derive(Debug, Clone)] > +pub struct VmEntry { > + pub vmid: u32, vmid and vmtype should also be aligned to snake-case > + pub vmtype: VmType, > + pub node: String, > + /// Per-VM version counter (increments when this VM's config changes) > + pub version: u32, > +} > + > +/// Information about a cluster member > +/// > +/// This is a shared type used by both cluster and DFSM modules > +#[derive(Debug, Clone)] > +pub struct MemberInfo { > + pub node_id: u32, > + pub pid: u32, > + pub joined_at: u64, > +} > + > +/// Node synchronization info for DFSM state sync > +/// > +/// Used during DFSM synchronization to track which nodes have provided state > +#[derive(Debug, Clone)] > +pub struct NodeSyncInfo { > + pub node_id: u32, > + pub pid: u32, > + pub state: Option>, Does the state have a fixed size? Also can we add a doc comment? > + pub synced: bool, What does it mean if this is true/false? Please add a doc comment for this pub field. > +}