Re: [PATCH pve-cluster 03/10] rust: ffi: add C ABI staticlib for pmxcfs

[Robert Obkircher <r.obkircher@proxmox.com>]

On 18.09.26 16:42, Hannes Laimer wrote:
> pmxcfs is a C daemon, so the notification server needs a small C
> facing surface it can link statically. Three functions cover start,
> event emission and shutdown, and a log callback taking a syslog
> priority routes messages into the daemon's own logging. The emit call
> takes the memdb version of the mutation along, which becomes the
> sequence number clients see and resume from.
>
> Rust aborts the process when a panic reaches the C caller. That would
> take down /etc/pve because of a bug in an optional feature, so every
> entry point catches a panic at that boundary, and one only disables
> the notifier for the rest of the daemon's life. It also drops every
> connection, so clients notice, reconnect and log the outage rather
> than waiting on a socket that will never speak again.
>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  src/rust/Cargo.toml                         |   1 +
>  src/rust/pmxcfs-ffi/Cargo.toml              |  17 ++
>  src/rust/pmxcfs-ffi/include/pmxcfs-notify.h |  29 ++
>  src/rust/pmxcfs-ffi/src/lib.rs              | 309 ++++++++++++++++++++
>  4 files changed, 356 insertions(+)
>  create mode 100644 src/rust/pmxcfs-ffi/Cargo.toml
>  create mode 100644 src/rust/pmxcfs-ffi/include/pmxcfs-notify.h
>  create mode 100644 src/rust/pmxcfs-ffi/src/lib.rs
>
> diff --git a/src/rust/Cargo.toml b/src/rust/Cargo.toml
> index 6de0ab8..9324685 100644
> --- a/src/rust/Cargo.toml
> +++ b/src/rust/Cargo.toml
> @@ -1,5 +1,6 @@
>  [workspace]
>  members = [
> +    "pmxcfs-ffi",
>      "pmxcfs-notify",
>  ]
>  resolver = "3"
> diff --git a/src/rust/pmxcfs-ffi/Cargo.toml b/src/rust/pmxcfs-ffi/Cargo.toml
> new file mode 100644
> index 0000000..8264cde
> --- /dev/null
> +++ b/src/rust/pmxcfs-ffi/Cargo.toml
> @@ -0,0 +1,17 @@
> +[package]
> +name = "pmxcfs-ffi"
> +version = "0.1.0"
> +description = "C ABI bridge exposing pmxcfs-notify to the pmxcfs daemon"
> +authors.workspace = true
> +edition.workspace = true
> +license.workspace = true
> +homepage.workspace = true
> +rust-version.workspace = true
> +
> +[lib]
> +crate-type = ["staticlib"]
> +
> +[dependencies]
> +libc.workspace = true
> +log.workspace = true
> +pmxcfs-notify.workspace = true
> diff --git a/src/rust/pmxcfs-ffi/include/pmxcfs-notify.h b/src/rust/pmxcfs-ffi/include/pmxcfs-notify.h
> new file mode 100644
> index 0000000..811213d
> --- /dev/null
> +++ b/src/rust/pmxcfs-ffi/include/pmxcfs-notify.h
> @@ -0,0 +1,29 @@
> +#ifndef PMXCFS_NOTIFY_H
> +#define PMXCFS_NOTIFY_H
> +
> +#include <stdint.h>
> +#include <sys/types.h>
> +
> +enum pmxcfs_notify_type {
> +    PMXCFS_NOTIFY_CREATE = 0,
> +    PMXCFS_NOTIFY_WRITE = 1,
> +    PMXCFS_NOTIFY_MTIME = 2,
> +    PMXCFS_NOTIFY_RENAME = 3,
> +    PMXCFS_NOTIFY_DELETE = 4,
> +    PMXCFS_NOTIFY_MKDIR = 5,
> +    PMXCFS_NOTIFY_RESYNC = 6,
> +};
> +
> +/* priority is a syslog(3) level */
> +typedef void (*pmxcfs_notify_log_fn)(int priority, const char *msg);
> +
> +/* seq is the version of the last mutation so far, a client resuming at it is current */
> +int pmxcfs_notify_init(
> +    const char *socket_path, gid_t gid, pmxcfs_notify_log_fn log_cb, uint64_t seq
> +);
> +void pmxcfs_notify_emit(
> +    enum pmxcfs_notify_type type, uint64_t seq, const char *path, const char *to
> +);
> +void pmxcfs_notify_shutdown(void);
> +
> +#endif /* PMXCFS_NOTIFY_H */
> diff --git a/src/rust/pmxcfs-ffi/src/lib.rs b/src/rust/pmxcfs-ffi/src/lib.rs
> new file mode 100644
> index 0000000..f8a9629
> --- /dev/null
> +++ b/src/rust/pmxcfs-ffi/src/lib.rs
> @@ -0,0 +1,309 @@
> +//! C ABI for the pmxcfs daemon, declared in include/pmxcfs-notify.h.
> +//!
> +//! Rust aborts the process when a panic reaches an `extern "C"` frame,
> +//! which must never happen to pmxcfs. Every entry point therefore runs
> +//! under catch_unwind and a panic switches the notifier off for the rest
> +//! of the daemon's life instead of taking it down.
> +
> +use std::ffi::{CStr, CString, OsStr, c_char, c_int};
> +use std::os::unix::ffi::OsStrExt;
> +use std::panic::{AssertUnwindSafe, catch_unwind};
> +use std::path::PathBuf;
> +use std::sync::atomic::{AtomicBool, Ordering};
> +use std::sync::{Mutex, MutexGuard, OnceLock};
> +
> +use log::{Level, LevelFilter, Log, Metadata, Record};
> +use pmxcfs_notify::{Config, EventKind, Server};
> +
> +pub type LogFn = unsafe extern "C" fn(c_int, *const c_char);
> +
> +const TYPE_CREATE: c_int = 0;
> +const TYPE_WRITE: c_int = 1;
> +const TYPE_MTIME: c_int = 2;
> +const TYPE_RENAME: c_int = 3;
> +const TYPE_DELETE: c_int = 4;
> +const TYPE_MKDIR: c_int = 5;
> +const TYPE_RESYNC: c_int = 6;
> +const RING_SIZE: usize = 8192;
> +
> +static SERVER: Mutex<Option<Server>> = Mutex::new(None);
> +static DISABLED: AtomicBool = AtomicBool::new(false);
> +static LOG_CB: Mutex<Option<LogFn>> = Mutex::new(None);
> +static LOGGER: CLogger = CLogger;
> +static LOGGER_INSTALLED: OnceLock<()> = OnceLock::new();
> +
> +struct CLogger;
> +
> +impl Log for CLogger {
> +    fn enabled(&self, _: &Metadata) -> bool {
> +        true
> +    }
> +
> +    fn log(&self, record: &Record) {
> +        let Some(callback) = *lock(&LOG_CB) else {
> +            return;
> +        };
> +        let priority = match record.level() {
> +            Level::Error => libc::LOG_ERR,
> +            Level::Warn => libc::LOG_WARNING,
> +            Level::Info => libc::LOG_INFO,
> +            Level::Debug | Level::Trace => libc::LOG_DEBUG,
> +        };
> +        let msg = record.args().to_string().replace('\0', " ");
> +        let Ok(msg) = CString::new(msg) else {
> +            return;
> +        };
> +        unsafe { callback(priority, msg.as_ptr()) };
> +    }
> +
> +    fn flush(&self) {}
> +}
> +
> +fn lock<T>(mutex: &Mutex<T>) -> MutexGuard<'_, T> {
> +    mutex
> +        .lock()
> +        .unwrap_or_else(|poisoned| poisoned.into_inner())
> +}
> +
> +fn guard<R>(what: &str, fallback: R, f: impl FnOnce() -> R) -> R {
> +    if DISABLED.load(Ordering::SeqCst) {
> +        return fallback;
> +    }
> +    match catch_unwind(AssertUnwindSafe(f)) {
> +        Ok(result) => result,
> +        Err(payload) => {
> +            DISABLED.store(true, Ordering::SeqCst);
> +            let reason = payload
> +                .downcast_ref::<&str>()
> +                .map(|s| s.to_string())
> +                .or_else(|| payload.downcast_ref::<String>().cloned())
to_string and cloned are theoretically allowed to panic on allocation
failure. I think you could just map the &String to &str instead.

(not a full review, I just quickly skimmed this patch)
> [..]