Re: [pve-devel] [PATCH pve-cluster 01/15] pmxcfs-rs: add workspace and pmxcfs-api-types crate

["Kefu Chai" <k.chai@proxmox.com>]

On Fri Jan 23, 2026 at 10:17 PM CST, Samuel Rufinatscha wrote:
> Thanks for the series. I’ve started reviewing patches 1–6; sending
> notes for patch 1 first, and I’ll follow up with comments on the
> others once I’ve gone through them in more depth.

Hi Samuel, thanks for your review. replies inlined.
>
> comments inline
>
> On 1/6/26 3:25 PM, Kefu Chai wrote:
>> Initialize the Rust workspace for the pmxcfs rewrite project.
>> 
>> 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)
>> 
>> This is the foundation crate with no internal dependencies, only
>> requiring thiserror and libc. All other crates will depend on these
>> shared type definitions.
>> 
>> Signed-off-by: Kefu Chai <k.chai@proxmox.com>
>> ---
>>   src/pmxcfs-rs/Cargo.lock                  | 2067 +++++++++++++++++++++
>
> Following the .gitignore pattern in our other repos, Cargo.lock is
> ignored, so I’d suggest dropping it from the series.

dropped.

>
>>   src/pmxcfs-rs/Cargo.toml                  |   83 +
>>   src/pmxcfs-rs/pmxcfs-api-types/Cargo.toml |   19 +
>>   src/pmxcfs-rs/pmxcfs-api-types/README.md  |  105 ++
>>   src/pmxcfs-rs/pmxcfs-api-types/src/lib.rs |  152 ++
>>   5 files changed, 2426 insertions(+)
>>   create mode 100644 src/pmxcfs-rs/Cargo.lock
>>   create mode 100644 src/pmxcfs-rs/Cargo.toml
>>   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/lib.rs
>> 
>> diff --git a/src/pmxcfs-rs/Cargo.lock b/src/pmxcfs-rs/Cargo.lock
>
> [..]
>
>> +++ b/src/pmxcfs-rs/Cargo.toml
>> @@ -0,0 +1,83 @@
>> +# Workspace root for pmxcfs Rust implementation
>> +[workspace]
>> +members = [
>> +    "pmxcfs-api-types", # Shared types and error definitions
>> +]
>> +resolver = "2"
>> +
>> +[workspace.package]
>> +version = "9.0.6"
>> +edition = "2024"
>> +authors = ["Proxmox Support Team <support@proxmox.com>"]
>> +license = "AGPL-3.0"
>> +repository = "https://git.proxmox.com/?p=pve-cluster.git"
>> +rust-version = "1.85"
>> +
>> +[workspace.dependencies]
>
> Here we already declare workspace path deps for crates that aren’t
> present yet (pmxcfs-config, pmxcfs-memdb, ...). For bisectability,
> could we keep this patch minimal and add those workspace
> members/path deps in the patches where the crates are introduced?

restructured the commits to add the deps only when they are used.

>
>> +# Internal workspace dependencies
>> +pmxcfs-api-types = { path = "pmxcfs-api-types" }
>> +pmxcfs-config = { path = "pmxcfs-config" }
>> +pmxcfs-memdb = { path = "pmxcfs-memdb" }
>> +pmxcfs-dfsm = { path = "pmxcfs-dfsm" }
>> +pmxcfs-rrd = { path = "pmxcfs-rrd" }
>> +pmxcfs-status = { path = "pmxcfs-status" }
>> +pmxcfs-ipc = { path = "pmxcfs-ipc" }
>> +pmxcfs-services = { path = "pmxcfs-services" }
>> +pmxcfs-logger = { path = "pmxcfs-logger" }
>> +
>> +# Core async runtime
>> +tokio = { version = "1.35", features = ["full"] }
>> +tokio-util = "0.7"
>> +async-trait = "0.1"
>> +
>
> If the goal is to centrally pin external crate versions early, maybe
> limit [workspace.dependencies] here generally to the crates actually
> used by pmxcfs-api-types (thiserror, libc) and extend as new crates
> are added.

likewise.

>
>> +# Error handling
>> +anyhow = "1.0"
>> +thiserror = "1.0"
>> +
>> +# Logging and tracing
>> +tracing = "0.1"
>> +tracing-subscriber = { version = "0.3", features = ["env-filter"] }
>> +
>> +# Serialization
>> +serde = { version = "1.0", features = ["derive"] }
>> +serde_json = "1.0"
>> +bincode = "1.3"
>> +
>> +# Network and cluster
>> +bytes = "1.5"
>> +sha2 = "0.10"
>> +bytemuck = { version = "1.14", features = ["derive"] }
>> +
>> +# System integration
>> +libc = "0.2"
>> +nix = { version = "0.27", features = ["fs", "process", "signal", "user", "socket"] }
>> +users = "0.11"
>> +
>> +# Corosync/CPG bindings
>> +rust-corosync = "0.1"
>> +
>> +# Enum conversions
>> +num_enum = "0.7"
>> +
>> +# Concurrency primitives
>> +parking_lot = "0.12"
>> +
>> +# Utilities
>> +chrono = "0.4"
>> +futures = "0.3"
>> +
>> +# Development dependencies
>> +tempfile = "3.8"
>> +
>> +[workspace.lints.clippy]
>> +uninlined_format_args = "warn"
>> +
>> +[profile.release]
>> +lto = true
>> +codegen-units = 1
>> +opt-level = 3
>> +strip = true
>> +
>> +[profile.dev]
>> +opt-level = 1
>> +debug = true
>> 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 00000000..cdce7951
>> --- /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 00000000..da8304ae
>> --- /dev/null
>> +++ b/src/pmxcfs-rs/pmxcfs-api-types/README.md
>> @@ -0,0 +1,105 @@
>> +# pmxcfs-api-types
>> +
>> +**Shared Types and Error Definitions** for pmxcfs.
>> +
>> +This crate provides common types, error definitions, and message formats used across all pmxcfs crates. It serves as the "API contract" between different components.
>> +
>> +## Overview
>> +
>> +The crate contains:
>> +- **Error types**: `PmxcfsError` with errno mapping for FUSE
>> +- **Message types**: `FuseMessage`, `KvStoreMessage`, `ApplicationMessage`
>
> These types and the mentioned serialization helpers aren’t part of this 
> diff, could you re-check both README.md (and the commit message) so they 
> match?

sorry, this README was revised before the last refactory. fixed.

>
>> +- **Shared types**: `MemberInfo`, `NodeSyncInfo`
>> +- **Serialization**: C-compatible wire format helpers
>> +
>> +## 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 |
>> +|-------|-------|-------|
>> +| `NotFound` | `ENOENT` | 2 |
>> +| `PermissionDenied` | `EPERM` | 1 |
>> +| `AlreadyExists` | `EEXIST` | 17 |
>> +| `NotADirectory` | `ENOTDIR` | 20 |
>> +| `IsADirectory` | `EISDIR` | 21 |
>> +| `DirectoryNotEmpty` | `ENOTEMPTY` | 39 |
>> +| `FileTooLarge` | `EFBIG` | 27 |
>> +| `ReadOnlyFilesystem` | `EROFS` | 30 |
>> +| `NoQuorum` | `EACCES` | 13 |
>> +| `Timeout` | `ETIMEDOUT` | 110 |
>> +
>> +## Message Types
>> +
>> +### FuseMessage
>> +
>> +Filesystem operations broadcast through the cluster (via DFSM). Uses C-compatible wire format compatible with `dcdb.c`.
>> +
>> +### KvStoreMessage
>> +
>> +Status and metrics synchronization (via kvstore DFSM). Uses C-compatible wire format.
>> +
>> +### ApplicationMessage
>> +
>> +Wrapper for either FuseMessage or KvStoreMessage, used by DFSM to handle both filesystem and status messages with type safety.
>> +
>> +## Shared Types
>> +
>> +### MemberInfo
>> +
>> +Cluster member information.
>> +
>> +### NodeSyncInfo
>> +
>> +DFSM synchronization state.
>> +
>> +## 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<T, PmxcfsError>` type
>> +
>> +### Message Types
>> +
>> +**C Version (dcdb.h):**
>> +
>> +**Rust Version:**
>> +- Type-safe enums
>> +
>> +## Key Differences from C Implementation
>> +
>> +All message types have `serialize()` and `deserialize()` methods that produce byte-for-byte compatible formats with the C implementation.
>> +
>> +## Known Issues / TODOs
>> +
>> +### Missing Features
>> +- None identified
>> +
>> +### Compatibility
>> +- **Wire format**: 100% compatible with C implementation
>> +- **errno values**: Match POSIX standards
>> +- **Message types**: All C message types covered
>> +
>> +## References
>> +
>> +### C Implementation
>> +- `src/pmxcfs/cfs-utils.h` - Utility types and error codes
>> +- `src/pmxcfs/dcdb.h` - FUSE message types
>> +- `src/pmxcfs/status.h` - KvStore message types
>> +
>> +### Related Crates
>> +- **pmxcfs-dfsm**: Uses ApplicationMessage for cluster sync
>> +- **pmxcfs-memdb**: Uses PmxcfsError for database operations
>> +- **pmxcfs**: Uses FuseMessage for FUSE operations
>> 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 00000000..ae0e5eb0
>> --- /dev/null
>> +++ b/src/pmxcfs-rs/pmxcfs-api-types/src/lib.rs
>> @@ -0,0 +1,152 @@
>> +use thiserror::Error;
>> +
>> +/// Error types for pmxcfs operations
>> +#[derive(Error, Debug)]
>> +pub enum PmxcfsError {
>
> nit: the error related parts could be added into a dedicated error.rs
> module

thanks! extracted.

>
>> +    #[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("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 {
>> +            PmxcfsError::NotFound(_) => libc::ENOENT,
>> +            PmxcfsError::PermissionDenied => libc::EPERM,
>> +            PmxcfsError::AlreadyExists(_) => libc::EEXIST,
>> +            PmxcfsError::NotADirectory(_) => libc::ENOTDIR,
>> +            PmxcfsError::IsADirectory(_) => libc::EISDIR,
>> +            PmxcfsError::DirectoryNotEmpty(_) => libc::ENOTEMPTY,
>> +            PmxcfsError::InvalidArgument(_) => libc::EINVAL,
>> +            PmxcfsError::FileTooLarge => libc::EFBIG,
>> +            PmxcfsError::ReadOnlyFilesystem => libc::EROFS,
>> +            PmxcfsError::NoQuorum => libc::EACCES,
>> +            PmxcfsError::Timeout => libc::ETIMEDOUT,
>> +            PmxcfsError::Io(e) => match e.raw_os_error() {
>> +                Some(errno) => errno,
>> +                None => libc::EIO,
>> +            },
>> +            _ => libc::EIO,
>
> Please check with C implementation, but:
>
> "PermissionDenied" should likely map to EACCES rather than EPERM. In
> FUSE/POSIX, EACCES is the standard return for file permission blocks,
> whereas EPERM is usually for administrative restrictions
> (like ownership)
>
> "InvalidPath" maps better to EINVAL. EIO suggests a hardware/disk
> failure, whereas InvalidPath implies an argument issue
>
> Also, "Lock" should explicitly be mapped.
> EBUSY (resource busy / lock contention)
> or EDEADLK (deadlock) / EAGAIN depending on semantics
>
> In general, can we minimize the number of errors falling into the
> generic EIO branch?
>

indeed. the way how the errors were categorized was way too
coarse-grained. fixed accordingly.

>> +        }
>> +    }
>> +}
>> +
>> +/// Result type for pmxcfs operations
>> +pub type Result<T> = std::result::Result<T, PmxcfsError>;
>> +
>> +/// VM/CT types
>> +#[derive(Debug, Clone, Copy, PartialEq, Eq)]
>
> If this is used in wire contexts please add #[repr(u8)] to ensure a
> stable ABI.

it's not use in wire format. i removed assignment statements, as in 
this case, we don't need predictable values for these values -- they 
are only used in-memory comparisons as distinct identifiers.

>
>> +pub enum VmType {
>> +    Qemu = 1,
>> +    Lxc = 3,
>
> There’s a gap between values 1 -> 3: is 2 reserved?
> If so, maybe add a short comment.

it's not reserved. actually, it's OpenVZ which was not supported anymore.
see https://www.proxmox.com/en/about/company-details/press-releases/proxmox-ve-4-0-released
now that the specific values are not assigned to these enum values, we
don't need to keep it anymore. but a short comment was added anyway to 
explain that OpenVZ support was removed.

>
>> +}
>> +
>> +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,
>> +    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 nodeid: u32,
>
> We have "nodeid" here but "node_id" in MemberInfo, this should be
> aligned.

thanks for pointing this out! changed to "node_id".
>
>> +    pub pid: u32,
>> +    pub state: Option<Vec<u8>>,
>> +    pub synced: bool,
>> +}



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel