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

[Samuel Rufinatscha <s.rufinatscha@proxmox.com>]

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.

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.

>   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?

> +# 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.

> +# 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?

> +- **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

> +    #[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?

> +        }
> +    }
> +}
> +
> +/// 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.

> +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.

> +}
> +
> +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.

> +    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