Re: [PATCH pve-cluster 02/14 v2] pmxcfs-rs: add pmxcfs-api-types crate

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

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<T> 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 <k.chai@proxmox.com>
> ---
>   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<T> 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<T, PmxcfsError>` 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<T> = std::result::Result<T, PmxcfsError>;
> 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<Vec<u8>>,

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.

> +}