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 <[email protected]> >> --- >> 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 <[email protected]>"] >> +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 [email protected] https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
