zbus migration

[jbaublitz]

Related to #2574

Summary by CodeRabbit

• New Features

  • Migrated to modern D-Bus library for improved performance and async support.
  • Enhanced manager and pool interfaces with expanded API coverage.
  • Improved state management and real-time signal notifications for pool and filesystem operations.

• Bug Fixes

  • Fixed asynchronous event handling and signal propagation for storage operations.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[packit-as-a-service]

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

• sudo dnf install -y 'dnf*-command(copr)'
• dnf copr enable packit/stratis-storage-stratisd-3945-copr_pull
• And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

[jbaublitz]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[jbaublitz]

@mulkieran This is still happening on a new PR.

[coderabbitai]

Walkthrough

Massive migration from dbus/dbus-tree crates to zbus, introducing versioned D-Bus manager (R0–R9) and pool (R0–R9) interface implementations, updated type system with feature-gated D-Bus derives, refactored signal emission infrastructure, and adjusted engine signatures for filesystem creation to return Name types instead of string references.

Changes

Cohort / File(s)	Change Summary
Dependency Migration Cargo.toml	Removed dbus and dbus-tree dependencies; added zbus 5.12.0 with tokio features; updated dbus_enabled feature flag.
D-Bus Core Infrastructure src/dbus/consts.rs, src/dbus/mod.rs, src/dbus/macros.rs, src/dbus/types.rs	Added D-Bus constants (STRATIS_BASE_PATH, STRATIS_BASE_SERVICE), re-exports for Manager/UdevHandler/signal utilities, new handle_action macro for logging/signaling, and public D-Bus wrapper types (ManagerR2, ManagerR8, DbusErrorEnum) with serialization implementations.
Manager Interface Versions src/dbus/manager/mod.rs, src/dbus/manager/manager_3_0/*, src/dbus/manager/manager_3_1/* through src/dbus/manager/manager_3_9/*	Implemented 10 versioned manager interfaces (org.storage.stratis3.Manager.r0–r9) with lifecycle methods (register/unregister), D-Bus method handlers for key/pool/unlock operations, properties (version, locked_pools, stopped_pools), and re-exports of shared method implementations.
Manager Helper Methods src/dbus/manager/manager_3_0/methods.rs, src/dbus/manager/manager_3_0/props.rs, src/dbus/manager/manager_3_2/methods.rs, src/dbus/manager/manager_3_2/props.rs, src/dbus/manager/manager_3_4/methods.rs, src/dbus/manager/manager_3_6/methods.rs, src/dbus/manager/manager_3_8/methods.rs	Created async method handlers for pool lifecycle (create, destroy, start, stop), key management, filesystem registration/unregistration, state refresh, and property accessors delegating to engine.
Filesystem D-Bus Layer src/dbus/filesystem/filesystem_3_0/*, src/dbus/filesystem/filesystem_3_9/*, src/dbus/filesystem/mod.rs, src/dbus/filesystem/shared.rs	Introduced filesystem D-Bus interfaces with version-specific property handlers, registration/unregistration methods, and shared async helper filesystem_prop for retrieving filesystem metadata.
Pool Interface Versions src/dbus/pool/mod.rs, src/dbus/pool/pool_3_0/* through src/dbus/pool/pool_3_9/*	Implemented 10 versioned pool interfaces (org.storage.stratis3.pool.r0–r9) with lifecycle management, methods for filesystem/device/clevis/keyring operations, and properties for pool state.
Pool Helper Methods & Properties src/dbus/pool/pool_3_0/methods.rs, src/dbus/pool/pool_3_0/props.rs, src/dbus/pool/pool_3_1/props.rs, src/dbus/pool/pool_3_3/methods.rs, src/dbus/pool/pool_3_5/methods.rs, src/dbus/pool/pool_3_6/methods.rs, src/dbus/pool/pool_3_7/methods.rs, src/dbus/pool/pool_3_8/*	Created async method handlers for filesystem creation/destruction, device operations, metadata retrieval, token binding/unbinding, and property accessors (size, encryption, cache state, token slots).
Pool Shared Utilities src/dbus/pool/shared.rs	Implemented async helpers pool_prop, try_pool_prop, set_pool_prop for retrieving and manipulating pool objects via engine with error mapping to zbus errors.
D-Bus Signal & Utility Layer src/dbus/util.rs, src/dbus/udev.rs	Added tuple-option conversions, engine error translation to D-Bus codes, signal emission macro and async helpers for pool/manager/filesystem state changes (background/foreground signals, locked pools, stopped pools, cache state, token slots). Introduced UdevHandler for coordinating udev events with D-Bus pool registration.
Engine Type System Updates src/engine/types/keys.rs, src/engine/types/mod.rs, src/engine/types/actions.rs, src/engine/types/diff.rs	Added feature-gated D-Bus derives (Type, Value) for UUID wrappers, Name, KeyDescription, and UnlockMethod types; changed ClevisInfo JSON type from Value to serde_json::Value; added Clone derives to SetCreateAction and GrowAction; added total_physical_size field to StratPoolDiff.
Engine Pool/Filesystem Signatures src/engine/engine.rs, src/engine/mod.rs, src/engine/sim_engine/pool.rs, src/engine/strat_engine/pool/dispatch.rs, src/engine/strat_engine/pool/v1.rs, src/engine/strat_engine/pool/v2.rs	Updated Pool::create_filesystems signature to remove lifetime parameter, accept &str specs (no lifetime), and return SetCreateAction<(Name, FilesystemUuid, Sectors)> instead of string references; added total_physical_size field to StratPoolState and threaded through state diffs/dumps.
Error Handling Refactor src/stratis/errors.rs	Replaced Arc<dbus::Error> with zbus::Error in StratisError::Dbus variant; added DbusType(zbus::zvariant::Error) variant under dbus_enabled; updated From trait implementations.
IPC & Event Handling src/lib.rs, src/stratis/dm.rs, src/stratis/timer.rs, src/stratis/run.rs, src/stratis/ipc_support/dbus_support.rs, src/stratis/ipc_support/jsonrpc_support.rs	Renamed public module from dbus_api to dbus; replaced D-Bus sender-based event emission with direct background signal helpers in dm/timer threads; refactored IPC setup to use new create_dbus_handler and UdevHandler; removed polling loop in jsonrpc event handling.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Specific areas requiring close attention:

• Manager and Pool versioning hierarchy: Review the inheritance/delegation pattern across all 10 manager versions (R0–R9) and 10 pool versions (R0–R9) to ensure consistency in method signatures, error handling, and signal emission.
• D-Bus interface correctness: Verify that all #[interface] macros, method signatures, and property annotations (emits_changed_signal, etc.) are properly configured for zbus.
• Signal emission logic: Inspect send_pool_background_signals, send_pool_foreground_signals, and related signal helpers to ensure proper manager locking, path resolution, and signal parameter construction.
• Engine signature migration: Confirm that all callers of create_filesystems have been updated to handle the new SetCreateAction<(Name, ...)> return type and Name wrapping.
• Error type conversions: Validate that all engine_to_dbus_err_tuple conversions, From<zbus::Error> implementations, and error propagation paths are sound.
• Feature-gated type definitions: Verify that D-Bus-specific derives and conversions (Type, Value, From) are only applied when dbus_enabled is active.
• Concurrency and locking: Review usage of Lockable<Arc<RwLock<Manager>>> and locking patterns in signal handlers and D-Bus method invocations.
• State field additions: Confirm that total_physical_size additions to StratPoolState and StratPoolDiff are correctly integrated into state tracking and diff computation.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 37.66% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check	❓ Inconclusive	The title 'zbus migration' is vague and generic, using a high-level technology name without clarifying the main purpose or change.	Consider a more descriptive title such as 'Migrate from dbus to zbus library' or 'Replace dbus crate with zbus for D-Bus communication' to clearly indicate the migration's scope and impact.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (12)

src/dbus/manager/manager_3_8/methods.rs (1)

180-192: Complex nested option handling for unlock method.

The double tuple_to_option pattern on line 187 handles the nested (bool, (bool, u32)) structure. This is correct but could be clarified with a comment explaining the unwrapping levels.

🔎 Suggested documentation improvement

     let key_fd_option = tuple_to_option(key_fd);
 
     match handle_action!(
         engine
             .start_pool(
                 id,
+                // Unwrap outer option (is unlock_method present?) then inner option (specific method)
                 TokenUnlockMethod::from_options(
                     tuple_to_option(unlock_method_tuple).map(tuple_to_option)
                 ),
                 key_fd_option.as_ref().map(|fd| fd.as_raw_fd()),
                 false
             )
             .await
     )

src/dbus/pool/shared.rs (1)

33-51: pool_prop and try_pool_prop have identical implementations.

These two functions (lines 33-41 and 43-51) are currently identical. If they are intended to have different semantics (e.g., try_pool_prop might wrap certain fallible operations differently), consider differentiating them. Otherwise, one could be an alias to the other to avoid duplication.

If this is intentional scaffolding for future divergence, a brief comment explaining the intended difference would help maintainability.

🔎 If no difference is intended, consider consolidating

 pub async fn pool_prop<R>(
     engine: &Arc<dyn Engine>,
     uuid: PoolUuid,
     f: impl Fn(SomeLockReadGuard<PoolUuid, dyn Pool>) -> R,
 ) -> Result<R, Error> {
     let guard = get_pool(engine, uuid).await?;

     Ok(f(guard))
 }

-pub async fn try_pool_prop<R>(
-    engine: &Arc<dyn Engine>,
-    uuid: PoolUuid,
-    f: impl Fn(SomeLockReadGuard<PoolUuid, dyn Pool>) -> R,
-) -> Result<R, Error> {
-    let guard = get_pool(engine, uuid).await?;
-
-    Ok(f(guard))
-}
+// Alias for pool_prop - currently identical, kept for API symmetry
+pub use pool_prop as try_pool_prop;

src/dbus/pool/pool_3_8/methods.rs (1)

251-260: Consider extracting duplicated signal condition logic.

The signal emission condition token_slot.is_none() || (token_slot.is_some() && token_slot == low_ts) || is_right appears in multiple methods. This could be extracted into a helper function for consistency and maintainability.

🔎 Example helper function

fn should_emit_primary_signal(token_slot: Option<u32>, low_ts: Option<u32>, is_right: bool) -> bool {
    token_slot.is_none() || (token_slot.is_some() && token_slot == low_ts) || is_right
}

Also applies to: 326-336, 408-418, 485-495

src/dbus/udev.rs (1)

62-72: Redundant Ok(()) return after ? operator.

The register_pool function call with ? already returns () on success. The explicit Ok(()) at line 71 is redundant.

🔎 Simplified implementation

 pub async fn register_pool(&self, pool_uuid: PoolUuid) -> StratisResult<()> {
     register_pool(
         &self.engine,
         &self.connection,
         &self.manager,
         &self.counter,
         pool_uuid,
     )
-    .await?;
-    Ok(())
+    .await
 }

src/dbus/manager/manager_3_2/methods.rs (1)

179-179: Clarify the unreachable case comment.

The comment "!has_partially_constructed above" may be misleading since there's no explicit has_partially_constructed check in this function. The CleanedUp variant is indeed unreachable here because this method operates on pools that have an ObjectPath (i.e., fully started pools), and CleanedUp only applies to partially constructed pools that were never started. Consider updating the comment to clarify this reasoning.

Suggested comment improvement

-        Ok(StopAction::CleanedUp(_)) => unreachable!("!has_partially_constructed above"),
+        Ok(StopAction::CleanedUp(_)) => unreachable!("CleanedUp only applies to partially constructed pools; this method operates on fully started pools with an ObjectPath"),

src/dbus/pool/mod.rs (1)

57-176: Consider: Repetitive registration pattern could be macro-ified.

The 10 near-identical if let Err blocks for registering PoolR{0..9} interfaces could be consolidated using a macro. However, this is a large migration PR, and the current explicit approach is clear and works correctly. This refactor can be deferred to a follow-up.

src/dbus/manager/mod.rs (2)

45-142: The add_ methods handle edge cases correctly but have significant duplication.*

The logic for add_pool, add_filesystem, and add_blockdev is nearly identical, differing only in the HashMap types and error messages. While the defensive programming pattern is sound (handling idempotent adds and detecting inconsistent state), this could be abstracted into a generic helper.

🔎 Consider a generic helper to reduce duplication

fn add_entry<K, V>(
    path_to_uuid: &mut HashMap<OwnedObjectPath, V>,
    uuid_to_path: &mut HashMap<V, OwnedObjectPath>,
    path: &ObjectPath<'_>,
    uuid: V,
    entity_type: &str,
) -> StratisResult<()>
where
    V: Eq + std::hash::Hash + Copy + std::fmt::Display,
{
    match (path_to_uuid.get(path), uuid_to_path.get(&uuid)) {
        (Some(u), Some(p)) => {
            if uuid == *u && path == &p.as_ref() {
                Ok(())
            } else {
                Err(StratisError::Msg(format!(
                    "Attempted to add {entity_type} path {path}, UUID {uuid} but entry path {p}, UUID {u} already exists"
                )))
            }
        }
        // ... rest of pattern
    }
}

---

190-233: Graceful degradation pattern is appropriate, but consider using a loop.

The continue-on-failure pattern is correct for graceful degradation. However, the repetitive registration calls could be condensed.

🔎 Consider using a macro or array of registration functions

pub async fn register_manager(
    connection: &Arc<Connection>,
    engine: &Arc<dyn Engine>,
    manager: &Lockable<Arc<RwLock<Manager>>>,
    counter: &Arc<AtomicU64>,
) {
    let registrations: [(fn(...) -> _, &str); 10] = [
        (ManagerR0::register, "Manager.r0"),
        (ManagerR1::register, "Manager.r1"),
        // ... etc
    ];
    
    for (register_fn, name) in registrations {
        if let Err(e) = register_fn(engine, connection, manager, counter).await {
            warn!("Failed to register interface {name}: {e}");
        }
    }
    
    if let Err(e) = connection
        .object_server()
        .at(STRATIS_BASE_PATH, ObjectManager)
        .await
    {
        warn!("Failed to register ObjectManager at {STRATIS_BASE_PATH}: {e}");
    }
}

Note: This would require the register functions to have identical signatures, which they should given the relevant code snippets showing all versions have the same signature.

src/engine/types/keys.rs (1)

470-494: Prefer implementing From instead of Into.

Rust conventions prefer implementing From<&EncryptionInfo> for serde_json::Value rather than Into<serde_json::Value> for &EncryptionInfo. The Into trait is automatically derived when From is implemented, and From is more idiomatic.

🔎 Suggested refactor

-impl Into<serde_json::Value> for &EncryptionInfo {
-    fn into(self) -> serde_json::Value {
+impl From<&EncryptionInfo> for serde_json::Value {
+    fn from(info: &EncryptionInfo) -> Self {
-        let json = self
+        let json = info
             .encryption_infos
             .iter()
             .map(|(token_slot, mech)| {
                 (
                     token_slot.to_string(),
                     match mech {
                         UnlockMechanism::KeyDesc(kd) => {
                             serde_json::Value::from(kd.as_application_str())
                         }
                         UnlockMechanism::ClevisInfo((pin, config)) => {
                             serde_json::Value::from(vec![
                                 serde_json::Value::from(pin.to_owned()),
                                 config.to_owned(),
                             ])
                         }
                     },
                 )
             })
             .collect::<Map<_, _>>();
         serde_json::Value::from(json)
     }
 }

src/stratis/ipc_support/dbus_support.rs (1)

45-50: Single-branch select! could be simplified.

The select! macro with a single branch is functionally equivalent to just awaiting the handle directly. Unless additional branches are planned for future expansion, consider simplifying.

🔎 Optional simplification

-    select! {
-        res = &mut udev_handle => {
-            error!("The udev processing thread exited...");
-            res.map_err(StratisError::from)
-        }
+    let res = udev_handle.await;
+    error!("The udev processing thread exited...");
+    res.map_err(StratisError::from)
-    }

If additional event sources are expected to be added later (e.g., signal handlers, other IPC channels), keeping the select! structure would be appropriate.

src/dbus/pool/pool_3_0/methods.rs (1)

296-365: Acknowledge TODO: Blockdev registration and object path return pending.

Multiple TODO comments indicate that blockdev registration and returning actual blockdev object paths are not yet implemented. The function currently returns default_return even on success.

Would you like me to open an issue to track the blockdev registration implementation for add_data_devs_method?

src/dbus/util.rs (1)

108-204: Consider reducing repetitive signal dispatch code.

The signal dispatch functions repeat nearly identical send_signal! calls for each pool version (R0–R9). While functional, this pattern is error-prone (as shown by the bug on line 820) and adds significant code volume.

A more maintainable approach could use a helper macro or loop construct, though this would require careful design given the different type parameters. This is optional and could be deferred.

[jbaublitz]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[mulkieran]

Current testing repo outcomes: FAILED (failures=7, errors=2). stratis-storage/testing#333 should reduce failures by 1. It did.

[mulkieran]

bingo. Fixing the create_filesystems returns will fix a few more...but the rest are probably real signal issues.

[mulkieran]

Builds are succeeding everywhere, but tests are failing on rawhide, which is kind of normal.