zbus migration #3927
zbus migration #3927
Conversation
WalkthroughReplace libdbus with optional zbus and add an async D‑Bus subsystem: new zbus dependency, Manager tracking path↔UUID, ManagerR0..R9, Pool/Filesystem objects, UdevHandler-driven registration/event processing, background signal emitters, and change create_filesystems to return Name instead of &str. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as D-Bus Client
participant ZConn as zbus Connection
participant ManagerR as ManagerR* Object
participant Engine as Stratis Engine
participant Registrar as register_pool / register_filesystem
participant Manager as Manager (path↔UUID)
Client->>ZConn: Method call (CreatePool / StartPool / ...)
ZConn->>ManagerR: dispatch to ManagerR*.method
ManagerR->>Engine: engine.create_pool / start_pool / ...
Engine-->>ManagerR: CreateAction / StartAction
ManagerR->>Registrar: register_pool / register_filesystem
Registrar->>Manager: manager.add_pool / add_filesystem
Registrar-->>ManagerR: ObjectPath(s)
ManagerR->>ZConn: send_*_background_signals
ZConn-->>Client: Reply
sequenceDiagram
participant UdevSrc as udev
participant UdevH as UdevHandler
participant Engine as Stratis Engine
participant Registrar as register_pool
participant DBus as zbus Object Server
UdevSrc->>UdevH: deliver UdevEngineEvent
UdevH->>Engine: engine.handle_events()
Engine-->>UdevH: pool events
UdevH->>Registrar: register_pool(pool_uuid)
Registrar->>DBus: register PoolR* / FilesystemR*
DBus-->>Registrar: ObjectPath result
Registrar-->>UdevH: registered paths
UdevH->>UdevSrc: continue event loop
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/dbus/udev.rs (1)
27-27: Consider returning a Result fromregister_poolfor consistencyReturning
StratisResult<()>keeps error propagation uniform with other async methods in this module and the wider crate.- pub async fn register_pool(&self, _name: &Name, _uuid: PoolUuid, _pool: &dyn Pool) { } + pub async fn register_pool(&self, _name: &Name, _uuid: PoolUuid, _pool: &dyn Pool) -> StratisResult<()> { + // TODO: implement + Ok(()) + }src/dbus/mod.rs (1)
23-31: Nit: split the builder chain for readability and error contextBreaking the long expression improves backtraces and future logging.
- let connection = Arc::new(Builder::system()?.serve_at(consts::STRATIS_BASE_PATH, manager)?.name(consts::STRATIS_BASE_SERVICE)?.build().await?); + let builder = Builder::system()?; + let builder = builder.serve_at(consts::STRATIS_BASE_PATH, manager)?; + let builder = builder.name(consts::STRATIS_BASE_SERVICE)?; + let connection = Arc::new(builder.build().await?);Cargo.toml (1)
360-364: Remove unused vendor filter exclusion for libdbus-sysThe codebase uses
zbus(line 291:dbus_enabled = ["dep:zbus"]), which is a pure Rust D-Bus implementation that does not depend onlibdbus-sys. The vendor filter exclusion at line 364 serves no purpose and should be removed.src/stratis/ipc_support/dbus_support.rs (1)
37-40: Consider parallel pool registration for better performance.The current implementation registers pools sequentially, which may be slow with many pools. Consider using
futures::future::join_allor similar to parallelize the registration if there are no ordering constraints.Example refactor:
let pools = engine.pools().await; -for (pool_name, pool_uuid, pool) in pools.iter() { - udev.register_pool(pool_name, *pool_uuid, pool).await; -} +use futures::future::join_all; +join_all( + pools.iter() + .map(|(pool_name, pool_uuid, pool)| { + udev.register_pool(pool_name, *pool_uuid, pool) + }) +).await;Note: Only apply this optimization if
register_poolis safe to call concurrently and if there are no ordering requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Cargo.toml(2 hunks)src/dbus/consts.rs(1 hunks)src/dbus/manager/manager_3_9.rs(1 hunks)src/dbus/manager/mod.rs(1 hunks)src/dbus/mod.rs(1 hunks)src/dbus/signal.rs(1 hunks)src/dbus/types.rs(1 hunks)src/dbus/udev.rs(1 hunks)src/lib.rs(1 hunks)src/stratis/dm.rs(1 hunks)src/stratis/errors.rs(3 hunks)src/stratis/ipc_support/dbus_support.rs(3 hunks)src/stratis/run.rs(1 hunks)src/stratis/timer.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/dbus/signal.rs (2)
src/dbus/manager/manager_3_9.rs (1)
new(18-23)src/dbus/udev.rs (1)
new(19-25)
src/dbus/types.rs (1)
src/stratis/keys.rs (1)
engine(28-46)
src/dbus/udev.rs (2)
src/dbus/manager/manager_3_9.rs (1)
new(18-23)src/dbus/signal.rs (1)
new(18-23)
src/dbus/mod.rs (3)
src/dbus/manager/manager_3_9.rs (1)
new(18-23)src/dbus/signal.rs (1)
new(18-23)src/dbus/udev.rs (1)
new(19-25)
src/dbus/manager/manager_3_9.rs (2)
src/dbus/signal.rs (1)
new(18-23)src/dbus/udev.rs (1)
new(19-25)
src/stratis/ipc_support/dbus_support.rs (4)
src/dbus/mod.rs (1)
create_dbus_handlers(23-32)src/dbus_api/util.rs (1)
create_dbus_handlers(160-178)src/engine/strat_engine/engine.rs (2)
pools(201-203)pools(739-741)src/engine/engine.rs (1)
pools(507-507)
🔇 Additional comments (20)
src/stratis/errors.rs (3)
148-150: LGTM! Simpler error formatting.The direct formatting of
zbus::Erroris more idiomatic than the previousmessage().unwrap_or()pattern.
225-229: Consistent with the updatedDbusvariant.The
Fromimplementation correctly convertszbus::ErrorwithoutArcwrapping, matching the new variant definition.
54-54: No issues found—zbus::ErrorimplementsClone.zbus::Error implements Clone, so the
Dbusvariant inStratisErroris correct. The enum'sClonederivation will compile without issues. The selective use ofArcin other variants (e.g.,Serde,Crypt,Join) is consistent with the codebase pattern and does not conflict with this change.src/dbus/manager/mod.rs (1)
1-3: LGTM! Clean module structure.The module declaration and re-export follow standard Rust patterns.
src/stratis/dm.rs (1)
19-19: LGTM! Import path updated for module rename.The import correctly reflects the
dbus_apitodbusmodule rename.src/dbus/consts.rs (1)
5-6: LGTM! Standard D-Bus constants.The constants follow D-Bus naming conventions for service and object paths.
src/lib.rs (1)
41-42: Module renamed fromdbus_apitodbus.This is a public API change that aligns with the zbus migration. External consumers will need to update their imports.
src/stratis/timer.rs (1)
12-12: LGTM! Import path updated consistently.The import correctly reflects the module rename, consistent with other files.
src/stratis/run.rs (2)
25-25: LGTM! Import path updated.The import correctly reflects the module rename.
103-108: Thesetup()signature change is verified as intentional and correct.The D-Bus
setup()implementation no longer receives or requires the trigger/should_exit parameter. The design correctly separates shutdown coordination:
- udev_thread() receives
should_exitfor graceful shutdown via explicit signal (line 39, udev_monitor.rs)- D-Bus setup() tasks run until error via
select!(dbus_support.rs, lines 64-73), exiting on thread failure rather than explicit signal- trigger remains defined and used (line 167, run.rs) for shutdown notification, but only applies to udev_thread and other components
This architectural separation is sound—the D-Bus layer doesn't require explicit shutdown coordination via the trigger.
src/dbus/manager/manager_3_9.rs (2)
12-15: Unused fields suggest incomplete implementation.Both
_engineand_receiverare prefixed with_to suppress warnings, indicating they're not currently used. The interface only exposes aVersionproperty that doesn't require these fields.If this is a placeholder for future D-Bus methods, consider adding a TODO comment. Otherwise, if these fields aren't needed, they could be removed to simplify the implementation.
Are these fields intended for future D-Bus method implementations, or should they be removed?
26-33: LGTM! Clean zbus interface implementation.The D-Bus interface correctly exposes the Version property with appropriate change signaling (
constmeans the property never changes during the object's lifetime, which is correct for the version string).Cargo.toml (1)
291-292: Feature gate looks rightSwitching
dbus_enabledto["dep:zbus"]matches the zbus builder usage added elsewhere.src/dbus/types.rs (1)
11-13: Code is correct—KeyDescription properly implements serde traitsVerification confirms
KeyDescriptionatsrc/engine/types/keys.rs:675is derived with#[derive(Clone, Debug, Eq, Hash, PartialEq, Serialize, Deserialize)], satisfying the serialization requirement. TheTypeimpl insrc/dbus/types.rslines 11–13 correctly maps it to&Signature::Str. No further action needed.src/dbus/mod.rs (1)
30-31: Error conversion is correctly wired.Verification confirms the
From<zbus::Error>impl forStratisErrorexists at src/stratis/errors.rs:225 and is properly feature-gated. The?operators in the builder chain will work as intended. No changes needed.src/stratis/ipc_support/dbus_support.rs (5)
23-23: LGTM! Import path updated for zbus migration.The import path change from
dbus_apitodbusaligns with the module restructuring for the zbus migration.
29-33: LGTM! Function signature properly updated for async flow.The function is now async and the
triggerparameter has been removed. This aligns with the zbus migration where synchronization is handled through async/await patterns rather than broadcast channels.
35-35: LGTM! Handler creation properly awaited.The call to
create_dbus_handlerscorrectly uses async/await and propagates errors. The return value change from(connection, udev, tree)to(udev, signal)reflects the zbus architecture where connection handling is managed differently.
43-50: LGTM! Signal handling task properly implemented.The signal handling task correctly spawns an async task, awaits the signal processing, and includes appropriate error handling and logging.
64-73: D-Bus method calls are correctly handled through zbus's automatic interface processing.Verification confirms the implementation is sound:
- ManagerR9 is properly defined with
#[interface(name = "org.storage.stratis3.Manager.r9")]macro- Builder::system().serve_at() automatically registers and handles incoming D-Bus requests on the interface
- zbus spawns internal threads to process method calls without requiring explicit connection handling
- The connection is kept alive via Arc, and process_dbus_actions() returning pending() is the correct pattern for the service to remain running
The removal of the dedicated
conn_handleis justified and the code change is correct.
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
jbaublitz
force-pushed
the
issue-stratisd-2574
branch
2 times, most recently
from
0834ca3 to
da14c7d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
src/dbus/pool/pool_3_0/mod.rs (1)
7-7: Consider adding module-level documentation.The re-exports are appropriate and follow standard Rust patterns. However, adding a module-level doc comment explaining the purpose of the
pool_3_0namespace would improve maintainability.For example:
+//! D-Bus pool interface version 3.0. +//! +//! Provides property accessors for pool UUID and name. + mod props;src/dbus/manager/manager_3_8/mod.rs (1)
5-7: Re-export visibilityIf
create_pool_methodis only consumed intra-crate, preferpub(crate)to avoid leaking API surface.-mod methods; - -pub use methods::create_pool_method; +mod methods; + +pub(crate) use methods::create_pool_method;src/dbus/pool/mod.rs (2)
31-35: Memory ordering nit
fetch_addis only for generating a unique ID;Ordering::Relaxedis sufficient and clearer.- counter.fetch_add(1, Ordering::AcqRel), + counter.fetch_add(1, Ordering::Relaxed),
31-35: Path layout sanity checkConsider namespacing pools, e.g.,
"{BASE}/pools/{id}", to avoid collisions with manager or other object paths. Verify against your D‑Bus path scheme to ensure no conflicts.src/dbus/manager/manager_3_0/props.rs (1)
7-9: Version getter: OKSimple and correct. If this is only used to expose a property, keeping it as-is is fine.
If desired, make it
pub const VERSION_PROP: &str = VERSION;to avoid a function call.src/dbus/manager/manager_3_0/mod.rs (1)
5-7: Module wiring: OK; consider restricting visibilityIf
version_propis internal, preferpub(crate)to avoid public API expansion.-pub use props::version_prop; +pub(crate) use props::version_prop;src/stratis/ipc_support/jsonrpc_support.rs (1)
29-31: Good: drain pending events to batch workThe non-blocking drain reduces wakeups and improves throughput.
Consider logging the batch size and/or inserting a cooperative yield if batches can be very large:
trace!("Handling {} udev events", events.len()); // tokio::task::yield_now().await; // optional fairnesssrc/stratis/run.rs (1)
24-26: DBus handler integration: wiring looks correct
- Using
create_dbus_handlerand threadingconnectionthrough DM/timer is coherent.setup(engine, udev)underdbus_enabledmatches the new IPC path.For graceful shutdown symmetry with the udev thread, consider passing a shutdown signal into the D-Bus
setuppath (e.g., via aCancellationTokenor broadcast receiver) so it can exit cleanly on SIGINT instead of relying on runtime teardown.Please confirm that
dbus::ipc_support::setupexits on drop/shutdown in your environment, or if you'd like, I can sketch a small token-based stop mechanism.Also applies to: 100-107, 115-117, 124-127
src/dbus/manager/manager_3_9/mod.rs (1)
39-50: Simplify redundant Arc cloning.Line 44 uses
Arc::clone(&engine)whereengineis alreadyArc<dyn Engine>, making the clone operation redundant since you're cloning the Arc itself, not creating a new reference.Apply this diff to simplify:
pub async fn register( connection: &Arc<Connection>, engine: Arc<dyn Engine>, counter: Arc<AtomicU64>, ) -> Result<()> { - let manager = Self::new(Arc::clone(connection), Arc::clone(&engine), counter); + let manager = Self::new(Arc::clone(connection), engine, counter); connection .object_server() .at(consts::STRATIS_BASE_PATH, manager) .await?; Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
Cargo.toml(2 hunks)src/dbus/consts.rs(1 hunks)src/dbus/manager/manager_3_0/mod.rs(1 hunks)src/dbus/manager/manager_3_0/props.rs(1 hunks)src/dbus/manager/manager_3_8/methods.rs(1 hunks)src/dbus/manager/manager_3_8/mod.rs(1 hunks)src/dbus/manager/manager_3_9/mod.rs(1 hunks)src/dbus/manager/mod.rs(1 hunks)src/dbus/mod.rs(1 hunks)src/dbus/pool/mod.rs(1 hunks)src/dbus/pool/pool_3_0/mod.rs(1 hunks)src/dbus/pool/pool_3_0/props.rs(1 hunks)src/dbus/pool/pool_3_9/mod.rs(1 hunks)src/dbus/types.rs(1 hunks)src/dbus/udev.rs(1 hunks)src/dbus/util.rs(1 hunks)src/lib.rs(1 hunks)src/stratis/dm.rs(4 hunks)src/stratis/errors.rs(3 hunks)src/stratis/ipc_support/dbus_support.rs(1 hunks)src/stratis/ipc_support/jsonrpc_support.rs(2 hunks)src/stratis/run.rs(2 hunks)src/stratis/timer.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/dbus/consts.rs
- src/lib.rs
- src/dbus/pool/pool_3_9/mod.rs
- src/dbus/manager/mod.rs
- src/dbus/types.rs
- src/dbus/udev.rs
- Cargo.toml
- src/dbus/mod.rs
- src/dbus/pool/pool_3_0/props.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/dbus/manager/manager_3_8/methods.rs
🧬 Code graph analysis (10)
src/dbus/pool/pool_3_0/mod.rs (2)
src/dbus/pool/pool_3_0/props.rs (2)
name_prop(18-25)uuid_prop(14-16)src/dbus_api/pool/pool_3_0/mod.rs (1)
props(7-7)
src/stratis/run.rs (5)
src/dbus/mod.rs (1)
create_dbus_handler(27-41)src/stratis/ipc_support/dbus_support.rs (1)
setup(23-51)src/stratis/ipc_support/jsonrpc_support.rs (1)
setup(39-56)src/stratis/dm.rs (1)
dm_event_thread(31-99)src/stratis/timer.rs (1)
run_timers(63-74)
src/dbus/manager/manager_3_8/mod.rs (1)
src/dbus/manager/manager_3_8/methods.rs (1)
create_pool_method(29-125)
src/dbus/pool/mod.rs (2)
src/dbus/udev.rs (1)
register_pool(59-68)src/dbus/pool/pool_3_9/mod.rs (1)
register(35-45)
src/dbus/manager/manager_3_9/mod.rs (2)
src/dbus/manager/manager_3_0/props.rs (1)
version_prop(7-9)src/dbus/manager/manager_3_8/methods.rs (4)
create_pool_method(29-125)devs(43-43)key_desc(44-47)clevis_info(48-48)
src/stratis/ipc_support/dbus_support.rs (3)
src/stratis/ipc_support/jsonrpc_support.rs (1)
setup(39-56)src/engine/strat_engine/engine.rs (2)
pools(201-203)pools(739-741)src/engine/engine.rs (1)
pools(507-507)
src/stratis/timer.rs (1)
src/dbus/util.rs (2)
send_fs_background_signals(45-50)send_pool_background_signals(37-42)
src/dbus/manager/manager_3_8/methods.rs (2)
src/dbus/pool/mod.rs (1)
register_pool(23-42)src/dbus/util.rs (2)
engine_to_dbus_err_tuple(28-34)tuple_to_option(18-24)
src/dbus/manager/manager_3_0/mod.rs (1)
src/dbus/manager/manager_3_0/props.rs (1)
version_prop(7-9)
src/stratis/dm.rs (1)
src/dbus/util.rs (2)
send_fs_background_signals(45-50)send_pool_background_signals(37-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (33)
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.90.0, cargo)
- GitHub Check: checks (make -f Makefile build, 1.90.0, cargo)
- GitHub Check: checks (make -f Makefile test, 1.90.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build, 1.90.0, cargo)
- GitHub Check: checks (make -f Makefile docs-ci, 1.90.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min, 1.90.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-utils, 1.90.0, cargo)
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.90.0, cargo)
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: checks (make -f Makefile build-min, 1.90.0, cargo)
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.90.0, cargo)
- GitHub Check: checks (make -f Makefile build, 1.90.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min-no-systemd, 1.90.0, cargo)
- GitHub Check: checks (make -f Makefile docs-ci, 1.90.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.90.0, clippy)
- GitHub Check: checks (make -f Makefile test, 1.90.0, cargo)
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.90.0, cargo)
- GitHub Check: python-based-tests
- GitHub Check: stratis-min-cli-checks
- GitHub Check: tests-with-testing-repo (master)
🔇 Additional comments (11)
src/dbus/pool/pool_3_0/mod.rs (1)
1-5: LGTM!The license header and module structure are correct and follow standard Rust conventions.
src/dbus/manager/manager_3_8/mod.rs (1)
1-4: License header: OKHeader looks correct and consistent.
src/stratis/errors.rs (1)
53-56: LGTM! Clean migration to zbus error types.The migration from
Arc<dbus::Error>tozbus::Errorand the addition ofDbusType(zbus::zvariant::Error)are well-implemented. The Display formatting distinguishes between general D-Bus errors and type-specific errors, and the From implementations correctly map zbus error types to StratisError variants.Also applies to: 150-156, 231-242
src/stratis/ipc_support/dbus_support.rs (2)
34-43: Single-task event loop simplifies coordination.The refactored implementation replaces the previous multi-thread coordination with a single asynchronous udev event loop. This is cleaner than the previous channel-based approach, though note that errors in
process_udev_eventswill terminate the loop.Verify that terminating the udev loop on errors is the desired behavior, as this will cause the entire D-Bus support task to exit. Compare with src/stratis/ipc_support/jsonrpc_support.rs which continues running even after one task fails.
24-31: Verify that non-fatal pool registration is acceptable.The code shows deliberate, consistent error handling: pool registration failures are logged but don't block D-Bus initialization across all three call sites in the codebase. The
setup()function returnsStratisResult<()>, which can propagate errors, but explicitly catches registration failures and continues, logging "D-Bus API is available" unconditionally.This graceful degradation appears intentional—pools may register asynchronously via subsequent udev events. However, without design documentation or tests clarifying this behavior, confirm:
- Is D-Bus functional without immediately-registered pools?
- Does asynchronous registration via udev events provide sufficient coverage?
- Are partial pool registration failures acceptable in production?
src/dbus/manager/manager_3_9/mod.rs (1)
53-88: LGTM! Clean D-Bus interface implementation.The ManagerR9 interface correctly delegates
CreatePoolto the sharedcreate_pool_methodimplementation and exposes the version property. The interface name follows the established naming convention.src/dbus/manager/manager_3_8/methods.rs (1)
28-125: LGTM! Comprehensive error handling throughout.The
create_pool_methodimplementation handles all error cases appropriately:
- Input normalization (devs to slices, tuples to options)
- JSON parsing with proper error propagation
- Encryption info validation
- Tag spec parsing with custom error messages
- Pool registration after creation
The error handling consistently uses
engine_to_dbus_err_tupleand returns appropriate D-Bus error tuples.src/stratis/timer.rs (2)
17-42: Clean refactor to connection-based signaling, but relies on incomplete stubs.The migration from channel-based
DbusActionto directsend_pool_background_signalsandsend_fs_background_signalscalls simplifies the architecture. However, these helpers are currently stubs in src/dbus/util.rs (lines 36-50) that always returnOk(())without performing actual signaling.Verify that the stub implementations will be completed before this migration is finalized, as pool and filesystem state changes won't propagate to D-Bus clients until these functions are implemented.
60-74: LGTM! Updated function signatures align with new architecture.The
run_timersfunction correctly threads theConnectionthrough tocheck_pool_and_fs, maintaining the connection-based pattern introduced in this migration.src/stratis/dm.rs (2)
31-69: Connection-based refactoring matches timer.rs pattern, but uses incomplete stubs.The
dm_event_threadandprocess_dm_eventfunctions have been updated consistently with the timer.rs changes, replacing channel-based signaling with directsend_pool_background_signalsandsend_fs_background_signalscalls. The same caveat applies: these are currently stubs (src/dbus/util.rs lines 36-50).Same verification as timer.rs: confirm these stub implementations will be completed, as devicemapper events won't propagate pool/filesystem state to D-Bus until implemented.
71-99: LGTM! Event loop structure correctly threads connection.The async event processing loop properly passes the connection reference to
process_dm_event, maintaining the new architecture's pattern.
jbaublitz
force-pushed
the
issue-stratisd-2574
branch
2 times, most recently
from
3c63993 to
859e831
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/dbus/pool/mod.rs (1)
29-57: Critical: Lifetime safety violation - return OwnedObjectPath instead.The
format!()call on lines 36-40 creates a temporaryStringthat is dropped after theletstatement completes, butObjectPath::try_fromborrows from this temporary. ReturningObjectPath<'a>with a lifetime tied to dropped data is undefined behavior.The function already correctly uses
OwnedObjectPathfor internal storage (line 54), but must also return owned types.Apply this fix (as suggested in previous review):
-pub async fn register_pool<'a>( +pub async fn register_pool( manager: &Lockable<Arc<RwLock<Manager>>>, connection: &Arc<Connection>, counter: &Arc<AtomicU64>, engine: Arc<dyn Engine>, pool_uuid: PoolUuid, -) -> StratisResult<(ObjectPath<'a>, Vec<ObjectPath<'a>>)> { - let path = ObjectPath::try_from(format!( +) -> StratisResult<(OwnedObjectPath, Vec<OwnedObjectPath>)> { + let path_str = format!( "{}/{}", consts::STRATIS_BASE_PATH, counter.fetch_add(1, Ordering::AcqRel), - ))?; + ); + let path = ObjectPath::try_from(path_str.as_str())?; PoolR9::register( connection, - path.clone(), + path, engine, manager.clone(), Arc::clone(counter), pool_uuid, ) .await?; + let owned_path = OwnedObjectPath::try_from(path_str)?; manager .write() .await - .add_pool(pool_uuid, OwnedObjectPath::from(path.clone())); + .add_pool(pool_uuid, owned_path.clone()); - Ok((path, Vec::default())) + Ok((owned_path, Vec::default())) }Also update callers (e.g.,
manager_3_8::create_pool_method) to acceptOwnedObjectPathin their return types.
🧹 Nitpick comments (5)
src/dbus/manager/mod.rs (2)
17-22: Consider encapsulating HashMap fields.The three HashMaps are currently public, which exposes internal state. While the Manager is wrapped in
RwLock<Manager>elsewhere, consider providing read-only accessor methods instead of public fields. This would allow you to change the internal representation later without breaking the API.However, if direct field access is needed for iteration or bulk operations, the current design may be acceptable.
37-47: Consider returning removal status.The
remove_*methods currently ignore the return value fromHashMap::remove(), which returnsOption<V>. Depending on your use case, you might want to:
- Return a boolean indicating whether the entry existed
- Return the removed
OwnedObjectPathfor cleanup- Keep the current design if the caller doesn't need this information
src/dbus/udev.rs (1)
41-60: LGTM! Efficient event batching strategy.The method correctly:
- Waits for at least one event (blocking recv)
- Drains any additional pending events (non-blocking try_recv)
- Processes all events in a single batch via
engine.handle_events- Logs failures during pool registration but continues processing
The error message at line 44 could be improved: "was shut" → "was shut down"
Apply this diff to improve the error message:
- StratisError::Msg("Channel from udev handler to D-Bus handler was shut".to_string()) + StratisError::Msg("Channel from udev handler to D-Bus handler was shut down".to_string())src/dbus/pool/shared.rs (1)
34-46: Consider preserving conversion error details.The error mapping discards the underlying conversion failure reason. If feasible, include the conversion error details in the error message for better debuggability.
src/dbus/util.rs (1)
45-82: Consider refactoring duplicate pool lookups.Lines 54-58 and 67-71 contain duplicate pool UUID lookup logic. Consider extracting the pool path lookup and interface retrieval to reduce duplication.
Apply this pattern:
pub async fn send_pool_background_signals( manager: Lockable<Arc<RwLock<Manager>>>, connection: &Arc<Connection>, diffs: HashMap<PoolUuid, PoolDiff>, ) -> StratisResult<()> { let dbus = manager.read().await; for (uuid, diff) in diffs { + let pool_path = dbus.pools.get(&uuid).ok_or_else(|| { + StratisError::Msg(format!("No pool associated with UUID {uuid}")) + })?; + if diff.thin_pool.allocated_size.changed().is_some() { let iface_ref = connection .object_server() - .interface::<_, PoolR9>(dbus.pools.get(&uuid).ok_or_else(|| { - StratisError::Msg(format!("No pool associated with UUID {uuid}")) - })?) + .interface::<_, PoolR9>(pool_path) .await?; iface_ref .get_mut() .await .allocated_size_changed(iface_ref.signal_emitter()) .await?; } if diff.thin_pool.used.changed().is_some() { let iface_ref = connection .object_server() - .interface::<_, PoolR9>(dbus.pools.get(&uuid).ok_or_else(|| { - StratisError::Msg(format!("No pool associated with UUID {uuid}")) - })?) + .interface::<_, PoolR9>(pool_path) .await?; iface_ref .get_mut() .await .total_physical_used_changed(iface_ref.signal_emitter()) .await?; } } Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
Cargo.toml(2 hunks)src/dbus/consts.rs(1 hunks)src/dbus/filesystem/filesystem_3_0/mod.rs(1 hunks)src/dbus/filesystem/filesystem_3_0/props.rs(1 hunks)src/dbus/filesystem/filesystem_3_9/mod.rs(1 hunks)src/dbus/filesystem/mod.rs(1 hunks)src/dbus/filesystem/shared.rs(1 hunks)src/dbus/manager/manager_3_0/mod.rs(1 hunks)src/dbus/manager/manager_3_0/props.rs(1 hunks)src/dbus/manager/manager_3_8/methods.rs(1 hunks)src/dbus/manager/manager_3_8/mod.rs(1 hunks)src/dbus/manager/manager_3_9/mod.rs(1 hunks)src/dbus/manager/mod.rs(1 hunks)src/dbus/mod.rs(1 hunks)src/dbus/pool/mod.rs(1 hunks)src/dbus/pool/pool_3_0/mod.rs(1 hunks)src/dbus/pool/pool_3_0/props.rs(1 hunks)src/dbus/pool/pool_3_6/methods.rs(1 hunks)src/dbus/pool/pool_3_6/mod.rs(1 hunks)src/dbus/pool/pool_3_9/mod.rs(1 hunks)src/dbus/pool/shared.rs(1 hunks)src/dbus/types.rs(1 hunks)src/dbus/udev.rs(1 hunks)src/dbus/util.rs(1 hunks)src/engine/engine.rs(1 hunks)src/engine/mod.rs(1 hunks)src/engine/sim_engine/pool.rs(2 hunks)src/engine/strat_engine/pool/dispatch.rs(1 hunks)src/engine/strat_engine/pool/v1.rs(2 hunks)src/engine/strat_engine/pool/v2.rs(2 hunks)src/lib.rs(1 hunks)src/stratis/dm.rs(4 hunks)src/stratis/errors.rs(3 hunks)src/stratis/ipc_support/dbus_support.rs(1 hunks)src/stratis/ipc_support/jsonrpc_support.rs(2 hunks)src/stratis/run.rs(2 hunks)src/stratis/timer.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/dbus/pool/pool_3_0/mod.rs
- src/dbus/manager/manager_3_8/methods.rs
- src/stratis/ipc_support/dbus_support.rs
- src/dbus/manager/manager_3_0/props.rs
- src/dbus/consts.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
Repo: stratis-storage/stratisd PR: 3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/dbus/manager/manager_3_8/mod.rssrc/dbus/pool/pool_3_6/methods.rssrc/dbus/pool/pool_3_6/mod.rssrc/dbus/manager/mod.rssrc/engine/engine.rssrc/stratis/dm.rssrc/dbus/pool/shared.rssrc/engine/strat_engine/pool/dispatch.rssrc/dbus/manager/manager_3_9/mod.rssrc/engine/strat_engine/pool/v1.rssrc/dbus/pool/pool_3_0/props.rssrc/dbus/pool/pool_3_9/mod.rssrc/dbus/util.rssrc/dbus/pool/mod.rssrc/engine/strat_engine/pool/v2.rssrc/stratis/timer.rs
📚 Learning: 2025-09-05T14:32:42.528Z
Learnt from: jbaublitz
Repo: stratis-storage/stratisd PR: 3898
File: src/engine/sim_engine/pool.rs:934-948
Timestamp: 2025-09-05T14:32:42.528Z
Learning: In src/engine/sim_engine/pool.rs, the start_encrypt_pool method intentionally creates a SizedKeyMemory with 1 byte allocated but 0 length reported - this is to avoid NULL pointer issues while simulating an empty key slice in the sim engine.
Applied to files:
src/dbus/manager/manager_3_8/mod.rssrc/engine/engine.rssrc/engine/sim_engine/pool.rssrc/dbus/pool/pool_3_9/mod.rs
🧬 Code graph analysis (27)
src/dbus/manager/manager_3_8/mod.rs (1)
src/dbus/manager/manager_3_8/methods.rs (1)
create_pool_method(32-129)
src/dbus/filesystem/shared.rs (2)
src/dbus/filesystem/filesystem_3_9/mod.rs (1)
Name(62-64)src/dbus/pool/pool_3_9/mod.rs (1)
Name(94-96)
src/dbus/filesystem/filesystem_3_0/props.rs (2)
src/dbus/filesystem/filesystem_3_9/mod.rs (1)
Name(62-64)src/dbus/pool/pool_3_9/mod.rs (1)
Name(94-96)
src/dbus/pool/pool_3_6/methods.rs (1)
src/dbus/filesystem/mod.rs (1)
register_filesystem(28-49)
src/dbus/pool/pool_3_6/mod.rs (1)
src/dbus/pool/pool_3_6/methods.rs (1)
create_filesystems_method(25-129)
src/dbus/manager/mod.rs (1)
src/stratis/keys.rs (1)
engine(28-46)
src/dbus/filesystem/filesystem_3_9/mod.rs (3)
src/dbus/filesystem/filesystem_3_0/props.rs (2)
name_prop(13-15)uuid_prop(9-11)src/dbus/filesystem/shared.rs (1)
filesystem_prop(11-30)src/dbus/pool/pool_3_9/mod.rs (4)
new(37-51)register(53-65)Uuid(88-90)Name(94-96)
src/stratis/run.rs (4)
src/dbus/mod.rs (1)
create_dbus_handler(29-56)src/stratis/ipc_support/dbus_support.rs (1)
setup(23-51)src/stratis/dm.rs (1)
dm_event_thread(36-110)src/stratis/timer.rs (1)
run_timers(74-88)
src/engine/engine.rs (7)
src/engine/sim_engine/pool.rs (1)
create_filesystems(221-263)src/engine/strat_engine/pool/dispatch.rs (1)
create_filesystems(111-121)src/engine/strat_engine/pool/v1.rs (1)
create_filesystems(846-895)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)src/dbus_api/pool/pool_3_0/methods.rs (1)
create_filesystems(29-129)src/dbus_api/pool/pool_3_6/methods.rs (1)
create_filesystems(21-130)src/engine/shared.rs (1)
specs(248-256)
src/dbus/udev.rs (3)
src/dbus/pool/mod.rs (1)
register_pool(29-57)src/dbus/manager/manager_3_9/mod.rs (1)
new(29-41)src/dbus/pool/pool_3_9/mod.rs (1)
new(37-51)
src/stratis/dm.rs (1)
src/dbus/util.rs (4)
send_fs_background_signals(85-91)send_pool_background_signals(46-82)connection(54-56)connection(67-69)
src/dbus/types.rs (2)
src/stratis/keys.rs (1)
engine(28-46)src/dbus/pool/pool_3_9/mod.rs (1)
Name(94-96)
src/dbus/pool/shared.rs (1)
src/engine/engine.rs (1)
get_pool(487-490)
src/engine/strat_engine/pool/dispatch.rs (6)
src/engine/engine.rs (1)
create_filesystems(162-167)src/engine/sim_engine/pool.rs (1)
create_filesystems(221-263)src/engine/strat_engine/pool/v1.rs (1)
create_filesystems(846-895)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)src/dbus_api/pool/pool_3_0/methods.rs (1)
create_filesystems(29-129)src/engine/shared.rs (1)
specs(248-256)
src/dbus/manager/manager_3_9/mod.rs (2)
src/dbus/manager/manager_3_0/props.rs (1)
version_prop(7-9)src/dbus/manager/manager_3_8/methods.rs (4)
create_pool_method(32-129)devs(47-47)key_desc(48-51)clevis_info(52-52)
src/engine/strat_engine/pool/v1.rs (7)
src/engine/engine.rs (3)
create_filesystems(162-167)size(93-93)size(122-122)src/engine/sim_engine/pool.rs (2)
create_filesystems(221-263)new(58-77)src/engine/strat_engine/pool/dispatch.rs (1)
create_filesystems(111-121)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)src/dbus_api/pool/pool_3_0/methods.rs (1)
create_filesystems(29-129)src/engine/shared.rs (1)
specs(248-256)src/engine/types/actions.rs (3)
new(235-237)new(311-313)new(586-589)
src/dbus/filesystem/filesystem_3_0/mod.rs (1)
src/dbus/filesystem/filesystem_3_0/props.rs (2)
name_prop(13-15)uuid_prop(9-11)
src/stratis/errors.rs (2)
src/dbus_api/blockdev/blockdev_3_0/api.rs (9)
f(34-34)f(41-41)f(48-48)f(57-57)f(64-64)f(71-71)f(78-78)f(85-85)f(92-92)src/dbus_api/filesystem/filesystem_3_0/api.rs (3)
f(33-33)f(40-40)f(47-47)
src/dbus/manager/manager_3_0/mod.rs (1)
src/dbus/manager/manager_3_0/props.rs (1)
version_prop(7-9)
src/dbus/mod.rs (5)
src/dbus/util.rs (4)
connection(54-56)connection(67-69)send_fs_background_signals(85-91)send_pool_background_signals(46-82)src/dbus/manager/manager_3_9/mod.rs (2)
new(29-41)register(43-55)src/dbus/pool/pool_3_9/mod.rs (2)
new(37-51)register(53-65)src/dbus/udev.rs (1)
new(25-39)src/engine/structures/lock.rs (1)
new_shared(84-86)
src/engine/sim_engine/pool.rs (6)
src/engine/engine.rs (3)
create_filesystems(162-167)size(93-93)size(122-122)src/engine/strat_engine/pool/dispatch.rs (1)
create_filesystems(111-121)src/engine/strat_engine/pool/v1.rs (1)
create_filesystems(846-895)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)src/dbus_api/pool/pool_3_0/methods.rs (1)
create_filesystems(29-129)src/dbus_api/pool/pool_3_6/methods.rs (1)
create_filesystems(21-130)
src/dbus/pool/pool_3_0/props.rs (1)
src/dbus/util.rs (1)
option_to_tuple(28-33)
src/dbus/filesystem/mod.rs (3)
src/dbus/filesystem/filesystem_3_9/mod.rs (1)
register(38-49)src/dbus/manager/manager_3_9/mod.rs (1)
register(43-55)src/dbus/pool/pool_3_9/mod.rs (1)
register(53-65)
src/dbus/pool/pool_3_9/mod.rs (3)
src/dbus/pool/pool_3_0/props.rs (5)
allocated_prop(35-37)name_prop(18-21)size_prop(23-25)used_prop(27-33)uuid_prop(14-16)src/dbus/pool/pool_3_6/methods.rs (1)
create_filesystems_method(25-129)src/dbus/pool/shared.rs (2)
pool_prop(21-32)try_pool_prop(34-46)
src/dbus/pool/mod.rs (5)
src/dbus/udev.rs (1)
register_pool(62-72)src/dbus/util.rs (2)
connection(54-56)connection(67-69)src/dbus/filesystem/filesystem_3_9/mod.rs (1)
register(38-49)src/dbus/manager/manager_3_9/mod.rs (1)
register(43-55)src/dbus/pool/pool_3_9/mod.rs (1)
register(53-65)
src/engine/strat_engine/pool/v2.rs (2)
src/engine/shared.rs (1)
specs(248-256)src/engine/types/mod.rs (4)
new(158-160)new(257-259)new(290-302)new(355-357)
src/stratis/timer.rs (1)
src/dbus/util.rs (4)
send_fs_background_signals(85-91)send_pool_background_signals(46-82)connection(54-56)connection(67-69)
🔇 Additional comments (45)
src/dbus/pool/pool_3_0/props.rs (1)
1-37: LGTM! Clean property getter implementation.The property functions follow consistent patterns and correctly convert pool data to zbus-compatible types. The implementation aligns well with the zbus migration objectives.
src/engine/mod.rs (1)
18-18: LGTM: Clean public API addition.The addition of
SomeLockReadGuardto the public re-exports aligns with the D-Bus integration changes across the PR.src/lib.rs (1)
42-42: LGTM: Module rename aligns with D-Bus reorganization.This is a breaking change (public module renamed from
dbus_apitodbus), but it's expected and appropriate for this major zbus migration.src/dbus/pool/pool_3_6/mod.rs (1)
1-7: LGTM: Clean module structure.The module wiring correctly declares and re-exports
create_filesystems_methodfollowing standard patterns.src/dbus/filesystem/filesystem_3_0/mod.rs (1)
1-7: LGTM: Clean module structure.The module wiring correctly declares and re-exports
name_propanduuid_propproperty helpers following standard patterns.src/dbus/manager/manager_3_8/mod.rs (1)
1-7: LGTM: Clean module structure.The module wiring correctly declares and re-exports
create_pool_methodfollowing standard patterns.src/dbus/manager/manager_3_0/mod.rs (1)
1-7: LGTM: Clean module structure.The module wiring correctly declares and re-exports
version_propfollowing standard patterns.src/stratis/ipc_support/jsonrpc_support.rs (1)
5-5: LGTM: Improved event batching efficiency.The refactor from
futures::poll!()totry_recv()is cleaner and more idiomatic. The logic correctly:
- Awaits the first event (blocking)
- Drains any additional available events without blocking
- Processes the batch together
This improves efficiency and reduces dependencies on futures polling utilities.
Also applies to: 29-31
Cargo.toml (1)
233-236: Verified: zbus 5.12.0 is current and secure.zbus 5.12.0 is the latest stable version on crates.io with no known security advisories. The migration from dbus/dbus-tree to zbus is correct.
src/dbus/types.rs (1)
9-19: LGTM! Type implementations are correct.The D-Bus Type trait implementations correctly map KeyDescription, PoolUuid, and Name to string signatures, which is appropriate for D-Bus serialization of these identifier types.
src/dbus/filesystem/filesystem_3_0/props.rs (1)
9-15: LGTM! Property helpers are clean.The functions provide straightforward conversions for D-Bus properties. The unused parameters in
name_prop(prefixed with_) are correctly marked as intentionally unused, likely required by the callback signature pattern.src/dbus/filesystem/shared.rs (1)
11-30: LGTM! Well-designed property accessor.The
filesystem_prophelper follows a clean callback pattern with proper error handling and descriptive error messages. The generic design allows flexible property extraction while maintaining type safety.src/engine/sim_engine/pool.rs (1)
221-226: LGTM! Signature change aligns with API migration to Name type.The updated
create_filesystemssignature removes the lifetime parameter and returnsNameinstead of&str, improving type safety and ownership clarity. The implementation correctly constructsNameinstances for the return values.Also applies to: 258-258
src/engine/engine.rs (1)
162-167: LGTM! Trait signature improvement.The updated
Pool::create_filesystemssignature removes lifetime complexity and returns ownedNametypes instead of borrowed string slices, improving API ergonomics and type safety across all pool implementations.src/stratis/run.rs (1)
100-131: LGTM! D-Bus infrastructure migration is well-structured.The transition from channel-based (
dbus_sender/dbus_receiver) to connection-based D-Bus handling viacreate_dbus_handleris correctly implemented with appropriatecfgguards for thedbus_enabledfeature. The connection and manager are properly threaded through todm_event_threadandrun_timers.src/dbus/pool/pool_3_6/methods.rs (1)
25-129: LGTM! Well-structured async D-Bus method implementation.The
create_filesystems_methodcorrectly handles:
- Parsing and validation of filesystem specifications
- Async/sync boundary via
spawn_blockingfor the engine call- Comprehensive error handling with appropriate D-Bus error codes
- Partial success scenarios (continues registering filesystems even if some registrations fail)
The implementation follows best practices for async Rust and D-Bus integration.
src/dbus/filesystem/mod.rs (1)
28-49: LGTM! Clean filesystem registration flow.The function correctly:
- Generates a unique D-Bus object path using an atomic counter
- Registers the filesystem with the D-Bus object server
- Updates the shared Manager state
- Follows the same pattern as pool registration
src/dbus/udev.rs (1)
62-72: LGTM! Clean delegation pattern.The method appropriately delegates to the module-level
register_poolfunction and discards the return values that aren't needed in this context.src/dbus/mod.rs (1)
29-56: LGTM! Well-structured D-Bus initialization.The function correctly:
- Initializes shared state (counter and manager)
- Establishes the system D-Bus connection with the appropriate service name
- Registers the manager interface before creating the UdevHandler
- Returns all necessary handles for the caller
The initialization order is correct, and resource ownership is properly managed through
Arc.src/engine/strat_engine/pool/dispatch.rs (1)
111-121: LGTM! Improved API with Name type.The signature change:
- Removes lifetime parameter 'a, simplifying the API
- Changes return type from
(&'a str, FilesystemUuid, Sectors)to(Name, FilesystemUuid, Sectors)- Aligns with the updated trait definition and V1/V2 implementations
Using the
Nametype instead of string slices provides better type safety and clearer semantics.src/engine/strat_engine/pool/v1.rs (1)
845-895: LGTM! Consistent with API changes.The implementation correctly:
- Updates the signature to remove the lifetime parameter
- Changes the return type to use
Nameinstead of&'a str- Wraps the filesystem name in
Name::new(name.to_string())before returningThis aligns with the trait definition and matches the v2 implementation pattern.
src/dbus/filesystem/filesystem_3_9/mod.rs (2)
38-49: LGTM! Standard D-Bus registration pattern.The function follows the established pattern for registering D-Bus interfaces, matching the approach used in
PoolR9::register.
60-64: Verify OwnedValue return type for Name property.The
Nameproperty returnsResult<OwnedValue, Error>instead of a concrete type likeString. While this provides flexibility, it's worth verifying that:
- The D-Bus type signature is correct for clients
- The
filesystem_prophelper properly converts the result to the expected D-Bus typeBased on the relevant code snippet from
filesystem_3_0/props.rs, it appearsname_propreturns aStr<'a>which should be compatible withOwnedValue. This is likely correct, but consider documenting whyOwnedValueis used instead ofStringif there's a specific reason.src/stratis/errors.rs (3)
53-56: LGTM! Proper zbus error handling.The changes correctly:
- Replace
dbus::Errorwithzbus::Error(no Arc needed as zbus::Error is already cheap to clone)- Add a separate
DbusTypevariant forzvariant::Error- Maintain feature gating with
dbus_enabledThis provides clear separation between general D-Bus errors and type conversion errors.
149-156: LGTM! Clear error messages.The Display implementation provides clear, consistent error messages for both D-Bus error types.
230-242: LGTM! Proper error conversion traits.The
Fromimplementations enable seamless error propagation with the?operator for both zbus and zvariant errors. Feature gating is consistent throughout.src/dbus/pool/shared.rs (2)
11-19: LGTM!The pool retrieval logic is correct and follows idiomatic Rust patterns with proper error handling.
21-32: LGTM!Clean abstraction for property access with infallible
Fromconversion.src/dbus/util.rs (3)
18-25: LGTM!Standard and correct implementation of the D-Bus optional value pattern.
27-33: LGTM!Correct inverse of
tuple_to_option, following the standard D-Bus optional pattern.
35-43: LGTM!Correct error mapping with appropriate special-casing for
DmError::Core.src/stratis/dm.rs (3)
14-46: LGTM!Import and signature updates are correct and properly guarded by feature flags.
64-75: LGTM!Signal emission with appropriate error handling via logging. The
manager.clone()on line 67 is necessary for the subsequent call on line 72.
86-94: LGTM!Call site correctly updated with new parameters and proper feature guards.
src/dbus/pool/pool_3_9/mod.rs (4)
28-65: LGTM!Well-structured PoolR9 implementation with appropriate visibility and correct async registration pattern.
71-84: LGTM!Clean delegation to the shared
create_filesystems_methodimplementation.
86-90: LGTM!Correct implementation with appropriate
constsignal emission setting for immutable UUID.
92-114: LGTM!All properties correctly implemented with appropriate signal emission settings and conversion strategies.
src/engine/strat_engine/pool/v2.rs (1)
751-800: LGTM!The signature change removes lifetime complexity and uses the more type-safe
Namewrapper. The implementation correctly constructsNameinstances.src/dbus/manager/manager_3_9/mod.rs (3)
21-55: LGTM!Well-structured ManagerR9 with correct async registration and appropriate field composition.
62-86: LGTM!Clean delegation to shared implementation with all parameters correctly forwarded.
88-93: LGTM!Correct property implementation with appropriate const signal emission for the immutable version.
src/stratis/timer.rs (3)
7-31: LGTM!Import and signature changes correctly implement the migration to connection-based signaling.
39-48: LGTM!Signal emission correctly implemented with appropriate error handling consistent with dm.rs.
53-87: LGTM!All call sites correctly updated with new parameters and proper feature guards.
jbaublitz
force-pushed
the
issue-stratisd-2574
branch
2 times, most recently
from
a544ea3 to
904f647
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/dbus/pool/pool_3_6/methods.rs (1)
72-79: Avoid transient String allocation when calling create_filesystems
Namealready exposes the pool label as&str, so we can skip theto_string().as_str()round trip and avoid a tiny allocation on this hot path.- pool.create_filesystems( - name.to_string().as_str(), + pool.create_filesystems( + name.as_ref(), pool_uuid, filesystem_specs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
Cargo.toml(2 hunks)src/dbus/consts.rs(1 hunks)src/dbus/filesystem/filesystem_3_0/mod.rs(1 hunks)src/dbus/filesystem/filesystem_3_0/props.rs(1 hunks)src/dbus/filesystem/filesystem_3_9/mod.rs(1 hunks)src/dbus/filesystem/mod.rs(1 hunks)src/dbus/filesystem/shared.rs(1 hunks)src/dbus/manager/manager_3_0/mod.rs(1 hunks)src/dbus/manager/manager_3_0/props.rs(1 hunks)src/dbus/manager/manager_3_8/methods.rs(1 hunks)src/dbus/manager/manager_3_8/mod.rs(1 hunks)src/dbus/manager/manager_3_9/mod.rs(1 hunks)src/dbus/manager/mod.rs(1 hunks)src/dbus/mod.rs(1 hunks)src/dbus/pool/mod.rs(1 hunks)src/dbus/pool/pool_3_0/mod.rs(1 hunks)src/dbus/pool/pool_3_0/props.rs(1 hunks)src/dbus/pool/pool_3_6/methods.rs(1 hunks)src/dbus/pool/pool_3_6/mod.rs(1 hunks)src/dbus/pool/pool_3_9/mod.rs(1 hunks)src/dbus/pool/shared.rs(1 hunks)src/dbus/types.rs(1 hunks)src/dbus/udev.rs(1 hunks)src/dbus/util.rs(1 hunks)src/engine/engine.rs(1 hunks)src/engine/mod.rs(1 hunks)src/engine/sim_engine/pool.rs(5 hunks)src/engine/strat_engine/pool/dispatch.rs(1 hunks)src/engine/strat_engine/pool/v1.rs(2 hunks)src/engine/strat_engine/pool/v2.rs(2 hunks)src/lib.rs(1 hunks)src/stratis/dm.rs(4 hunks)src/stratis/errors.rs(3 hunks)src/stratis/ipc_support/dbus_support.rs(1 hunks)src/stratis/ipc_support/jsonrpc_support.rs(2 hunks)src/stratis/run.rs(2 hunks)src/stratis/timer.rs(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/dbus/pool/pool_3_0/props.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- src/engine/mod.rs
- src/dbus/pool/pool_3_0/mod.rs
- src/dbus/filesystem/filesystem_3_0/mod.rs
- src/dbus/manager/manager_3_0/props.rs
- src/dbus/filesystem/filesystem_3_0/props.rs
- src/dbus/manager/mod.rs
- src/dbus/manager/manager_3_9/mod.rs
- src/dbus/pool/mod.rs
- src/dbus/util.rs
- src/lib.rs
- src/dbus/consts.rs
- src/dbus/pool/shared.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
Repo: stratis-storage/stratisd PR: 3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/engine/engine.rssrc/dbus/manager/manager_3_8/methods.rssrc/engine/strat_engine/pool/v2.rssrc/dbus/pool/pool_3_6/methods.rssrc/dbus/manager/manager_3_8/mod.rssrc/engine/strat_engine/pool/dispatch.rssrc/stratis/dm.rssrc/dbus/pool/pool_3_6/mod.rssrc/stratis/timer.rssrc/engine/strat_engine/pool/v1.rssrc/dbus/pool/pool_3_9/mod.rs
📚 Learning: 2025-09-05T14:32:42.528Z
Learnt from: jbaublitz
Repo: stratis-storage/stratisd PR: 3898
File: src/engine/sim_engine/pool.rs:934-948
Timestamp: 2025-09-05T14:32:42.528Z
Learning: In src/engine/sim_engine/pool.rs, the start_encrypt_pool method intentionally creates a SizedKeyMemory with 1 byte allocated but 0 length reported - this is to avoid NULL pointer issues while simulating an empty key slice in the sim engine.
Applied to files:
src/engine/engine.rssrc/dbus/manager/manager_3_8/methods.rssrc/dbus/manager/manager_3_8/mod.rssrc/dbus/pool/pool_3_9/mod.rssrc/engine/sim_engine/pool.rs
🧬 Code graph analysis (22)
src/engine/engine.rs (7)
src/engine/sim_engine/pool.rs (1)
create_filesystems(221-263)src/engine/strat_engine/pool/dispatch.rs (1)
create_filesystems(111-121)src/engine/strat_engine/pool/v1.rs (1)
create_filesystems(846-895)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)src/dbus_api/pool/pool_3_0/methods.rs (1)
create_filesystems(29-129)src/dbus_api/pool/pool_3_6/methods.rs (1)
create_filesystems(21-130)src/engine/shared.rs (1)
specs(248-256)
src/dbus/manager/manager_3_8/methods.rs (2)
src/dbus/pool/mod.rs (1)
register_pool(29-57)src/dbus/util.rs (4)
engine_to_dbus_err_tuple(37-43)tuple_to_option(19-25)connection(54-56)connection(67-69)
src/engine/strat_engine/pool/v2.rs (2)
src/engine/shared.rs (1)
specs(248-256)src/engine/types/mod.rs (4)
new(158-160)new(257-259)new(290-302)new(355-357)
src/dbus/pool/pool_3_6/methods.rs (2)
src/dbus/filesystem/mod.rs (1)
register_filesystem(28-49)src/dbus/filesystem/filesystem_3_9/mod.rs (2)
new(30-36)Uuid(56-58)
src/stratis/errors.rs (3)
src/dbus_api/blockdev/blockdev_3_0/api.rs (9)
f(34-34)f(41-41)f(48-48)f(57-57)f(64-64)f(71-71)f(78-78)f(85-85)f(92-92)src/dbus_api/pool/pool_3_1/api.rs (3)
f(17-17)f(28-28)f(39-39)src/dbus_api/filesystem/filesystem_3_0/api.rs (3)
f(33-33)f(40-40)f(47-47)
src/stratis/run.rs (5)
src/dbus/mod.rs (1)
create_dbus_handler(29-56)src/stratis/ipc_support/dbus_support.rs (1)
setup(23-51)src/stratis/ipc_support/jsonrpc_support.rs (1)
setup(39-56)src/stratis/dm.rs (1)
dm_event_thread(36-110)src/stratis/timer.rs (1)
run_timers(74-88)
src/dbus/udev.rs (3)
src/dbus/pool/mod.rs (1)
register_pool(29-57)src/dbus/manager/manager_3_9/mod.rs (1)
new(29-41)src/dbus/pool/pool_3_9/mod.rs (1)
new(37-51)
src/stratis/ipc_support/dbus_support.rs (3)
src/stratis/ipc_support/jsonrpc_support.rs (1)
setup(39-56)src/engine/engine.rs (1)
pools(507-507)src/engine/strat_engine/engine.rs (2)
pools(201-203)pools(739-741)
src/dbus/manager/manager_3_8/mod.rs (1)
src/dbus/manager/manager_3_8/methods.rs (1)
create_pool_method(32-129)
src/engine/strat_engine/pool/dispatch.rs (5)
src/engine/engine.rs (1)
create_filesystems(162-167)src/engine/sim_engine/pool.rs (1)
create_filesystems(221-263)src/engine/strat_engine/pool/v1.rs (1)
create_filesystems(846-895)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)src/engine/shared.rs (1)
specs(248-256)
src/stratis/dm.rs (1)
src/dbus/util.rs (4)
send_fs_background_signals(85-91)send_pool_background_signals(46-82)connection(54-56)connection(67-69)
src/dbus/filesystem/shared.rs (2)
src/dbus/filesystem/filesystem_3_9/mod.rs (2)
Name(62-64)Uuid(56-58)src/dbus/pool/pool_3_9/mod.rs (2)
Name(94-96)Uuid(88-90)
src/dbus/manager/manager_3_0/mod.rs (1)
src/dbus/manager/manager_3_0/props.rs (1)
version_prop(7-9)
src/dbus/pool/pool_3_6/mod.rs (1)
src/dbus/pool/pool_3_6/methods.rs (1)
create_filesystems_method(25-129)
src/stratis/timer.rs (1)
src/dbus/util.rs (4)
send_fs_background_signals(85-91)send_pool_background_signals(46-82)connection(54-56)connection(67-69)
src/dbus/mod.rs (6)
src/dbus/util.rs (4)
connection(54-56)connection(67-69)send_fs_background_signals(85-91)send_pool_background_signals(46-82)src/dbus/filesystem/filesystem_3_9/mod.rs (2)
new(30-36)register(38-49)src/dbus/manager/manager_3_9/mod.rs (2)
new(29-41)register(43-55)src/dbus/pool/pool_3_9/mod.rs (2)
new(37-51)register(53-65)src/dbus/udev.rs (1)
new(25-39)src/engine/structures/lock.rs (1)
new_shared(84-86)
src/engine/strat_engine/pool/v1.rs (6)
src/engine/engine.rs (3)
create_filesystems(162-167)size(93-93)size(122-122)src/engine/sim_engine/pool.rs (2)
create_filesystems(221-263)new(58-77)src/engine/strat_engine/pool/dispatch.rs (1)
create_filesystems(111-121)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)src/dbus_api/pool/pool_3_0/methods.rs (1)
create_filesystems(29-129)src/engine/shared.rs (1)
specs(248-256)
src/dbus/pool/pool_3_9/mod.rs (3)
src/dbus/pool/pool_3_0/props.rs (5)
allocated_prop(35-37)name_prop(18-21)size_prop(23-25)used_prop(27-33)uuid_prop(14-16)src/dbus/pool/pool_3_6/methods.rs (1)
create_filesystems_method(25-129)src/dbus/pool/shared.rs (2)
pool_prop(21-32)try_pool_prop(34-46)
src/dbus/types.rs (2)
src/stratis/keys.rs (1)
engine(28-46)src/dbus/pool/pool_3_9/mod.rs (1)
Name(94-96)
src/dbus/filesystem/mod.rs (3)
src/dbus/filesystem/filesystem_3_9/mod.rs (1)
register(38-49)src/dbus/manager/manager_3_9/mod.rs (1)
register(43-55)src/dbus/pool/pool_3_9/mod.rs (1)
register(53-65)
src/engine/sim_engine/pool.rs (3)
src/engine/engine.rs (1)
create_filesystems(162-167)src/engine/types/actions.rs (3)
new(235-237)new(311-313)new(586-589)src/engine/types/mod.rs (6)
new(158-160)new(257-259)new(290-302)new(355-357)as_ref(168-170)as_ref(392-397)
src/dbus/filesystem/filesystem_3_9/mod.rs (2)
src/dbus/filesystem/filesystem_3_0/props.rs (2)
name_prop(13-15)uuid_prop(9-11)src/dbus/filesystem/shared.rs (1)
filesystem_prop(11-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.91.0, clippy)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile stratisd-tools, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-no-ipc, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-utils, 1.91.0, cargo)
- GitHub Check: python-based-tests
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
- GitHub Check: stratis-min-cli-checks
🔇 Additional comments (16)
src/dbus/manager/manager_3_0/mod.rs (1)
1-7: LGTM!The module structure is clean and follows Rust conventions. The separation between the public facade (
mod.rs) and the internal implementation (props.rs) provides good organization and allows for future extensibility as more properties are added to the manager interface.src/dbus/filesystem/shared.rs (1)
11-30: Shared helper makes filesystem property lookups tidyNice job consolidating the common guard/lookup/error pattern into one async helper—this keeps the per-property call sites lean while preserving precise error reporting.
src/engine/strat_engine/pool/v1.rs (2)
846-851: LGTM: Signature change aligns with Name type migration.The removal of the lifetime parameter and switch from
&'a strtoNamein the return type is consistent with the broader codebase migration seen in v2.rs, dispatch.rs, and engine.rs.
890-890: LGTM: Correct Name wrapping.The conversion from
&strtoStringviato_string()before wrapping inName::new()is necessary and consistent with the same pattern in v2.rs.src/dbus/udev.rs (2)
41-60: LGTM: Event processing correctly implemented.The method properly:
- Awaits the first event and handles channel closure with an appropriate error message
- Drains additional events using
try_recv()to batch process- Delegates event handling to the engine
- Logs registration failures without stopping the loop
This addresses the previous review comment about replacing
pending().awaitwith actual event processing.
62-72: LGTM: Clean delegation to pool registration.The method correctly passes all required parameters to the
register_poolfunction and propagates the result.src/dbus/mod.rs (1)
29-56: LGTM: D-Bus handler creation properly orchestrated.The function correctly:
- Initializes shared state (counter, manager) before D-Bus connection
- Creates and configures the system D-Bus connection with the service name
- Registers ManagerR9 at the base path before creating UdevHandler
- Returns all necessary components for the caller to manage
The Arc cloning pattern ensures proper shared ownership across components.
src/engine/strat_engine/pool/dispatch.rs (1)
111-121: LGTM: Dispatch correctly updated for Name type.The signature change is consistent with the v1 and v2 implementations, properly removing the lifetime parameter and returning
Nameinstead of&'a str.src/dbus/filesystem/filesystem_3_9/mod.rs (2)
23-50: LGTM: FilesystemR9 registration follows established pattern.The struct and registration function follow the same pattern as PoolR9 and ManagerR9, with proper Arc cloning and async registration.
52-65: LGTM: D-Bus interface properties correctly defined.The properties are appropriately marked with
emits_changed_signal = "const"since filesystem UUID and name are immutable after creation. The use of shared utility functions ensures consistency across interface versions.src/stratis/errors.rs (3)
54-56: LGTM: Error variants updated for zbus migration.The change from
Arc<dbus::Error>tozbus::Errorand addition ofDbusType(zbus::zvariant::Error)correctly reflects the zbus library's separation of general D-Bus errors and type conversion errors. The Arc removal is appropriate as zbus::Error has different ownership semantics.
150-156: LGTM: Display implementation updated for new error variants.The format strings appropriately distinguish between general D-Bus errors and type conversion errors.
231-241: LGTM: From implementations correctly map zbus errors.The trait implementations properly convert both zbus error types to their corresponding StratisError variants, correctly gated behind the dbus_enabled feature.
src/dbus/manager/manager_3_8/methods.rs (1)
32-129: LGTM: Pool creation method handles complex input validation correctly.The function properly:
- Normalizes and validates all input parameters (paths, encryption info, integrity specs)
- Handles JSON parsing errors for Clevis info with appropriate error mapping
- Constructs the IntegritySpec from optional components
- Delegates to the engine and registers the pool on success
- Returns appropriate D-Bus response tuples for all cases (created, identity, error)
The error handling at each validation step ensures invalid inputs are caught early with meaningful error messages.
src/dbus/pool/pool_3_9/mod.rs (2)
28-66: LGTM: PoolR9 registration follows established pattern.The struct and registration function are consistent with other R9 interface objects (FilesystemR9, ManagerR9), with proper Arc cloning and async registration at the provided D-Bus path.
68-115: LGTM: D-Bus interface correctly implements pool operations and properties.The interface properly:
- Delegates
CreateFilesystemsto the shared method implementation with all required parameters- Marks UUID and Name as const (immutable after creation)
- Marks physical size properties with
emits_changed_signal = "true"(can change when pool grows or shrinks)- Uses shared utility functions to fetch property values consistently
jbaublitz
force-pushed
the
issue-stratisd-2574
branch
from
362d7be to
904f647
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/dbus/pool/pool_3_6/methods.rs (1)
72-79: Avoid cloning the pool name at call timeWe already borrow the pool name from the guard, so cloning it just to pass a temporary
&strallocates on every filesystem creation. Borrow the existing string instead.- pool.create_filesystems( - name.to_string().as_str(), + pool.create_filesystems( + name.as_str(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/dbus/pool/pool_3_6/methods.rs(1 hunks)src/dbus/pool/pool_3_9/methods.rs(1 hunks)src/dbus/pool/pool_3_9/mod.rs(1 hunks)src/dbus/pool/pool_3_9/props.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
Repo: stratis-storage/stratisd PR: 3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/dbus/pool/pool_3_9/methods.rssrc/dbus/pool/pool_3_6/methods.rssrc/dbus/pool/pool_3_9/mod.rssrc/dbus/pool/pool_3_9/props.rs
📚 Learning: 2025-09-05T14:32:42.528Z
Learnt from: jbaublitz
Repo: stratis-storage/stratisd PR: 3898
File: src/engine/sim_engine/pool.rs:934-948
Timestamp: 2025-09-05T14:32:42.528Z
Learning: In src/engine/sim_engine/pool.rs, the start_encrypt_pool method intentionally creates a SizedKeyMemory with 1 byte allocated but 0 length reported - this is to avoid NULL pointer issues while simulating an empty key slice in the sim engine.
Applied to files:
src/dbus/pool/pool_3_9/methods.rssrc/dbus/pool/pool_3_9/mod.rs
🧬 Code graph analysis (3)
src/dbus/pool/pool_3_9/methods.rs (2)
src/dbus/manager/manager_3_8/methods.rs (1)
from_str(59-59)src/stratis/keys.rs (1)
engine(28-46)
src/dbus/pool/pool_3_6/methods.rs (1)
src/dbus/filesystem/mod.rs (1)
register_filesystem(28-49)
src/dbus/pool/pool_3_9/mod.rs (5)
src/dbus/pool/pool_3_0/props.rs (5)
allocated_prop(35-37)name_prop(18-21)size_prop(23-25)used_prop(27-33)uuid_prop(14-16)src/dbus/pool/pool_3_6/methods.rs (1)
create_filesystems_method(25-129)src/dbus/pool/pool_3_9/methods.rs (1)
encrypt_pool_method(25-139)src/dbus/pool/pool_3_9/props.rs (1)
last_reencrypted_timestamp_prop(13-22)src/dbus/pool/shared.rs (2)
pool_prop(21-32)try_pool_prop(34-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: checks (make -f Makefile fmt-ci, 1.91.0, rustfmt)
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-utils, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile docs-ci, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.91.0, clippy)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile stratisd-tools, 1.91.0, cargo)
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile test, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-no-ipc, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile build-min, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile build, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min-no-systemd, 1.91.0, cargo)
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.91.0, cargo)
🔇 Additional comments (3)
src/dbus/pool/pool_3_9/props.rs (1)
13-21: Helper correctly wraps optional timestampFormatting the optional timestamp to RFC 3339 and feeding it through
option_to_tuplemirrors the established property pattern; looks good.src/dbus/pool/pool_3_9/methods.rs (1)
24-138: Async encrypt flow looks consistentThe staged blocking operations, error translation, and return contract line up with the existing engine semantics; nothing further from my side.
src/dbus/pool/pool_3_9/mod.rs (1)
32-140: PoolR9 wiring matches the new helpersRegistration, method delegation, and property exposure are consistent with the shared helpers and earlier versions; looks good.
jbaublitz
force-pushed
the
issue-stratisd-2574
branch
from
904f647 to
c63b1e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
Cargo.toml(2 hunks)src/dbus/filesystem/filesystem_3_0/mod.rs(1 hunks)src/dbus/filesystem/filesystem_3_0/props.rs(1 hunks)src/dbus/filesystem/filesystem_3_9/mod.rs(1 hunks)src/dbus/filesystem/mod.rs(1 hunks)src/dbus/filesystem/shared.rs(1 hunks)src/dbus/manager/manager_3_0/methods.rs(1 hunks)src/dbus/manager/manager_3_0/mod.rs(1 hunks)src/dbus/manager/manager_3_9/mod.rs(1 hunks)src/dbus/manager/mod.rs(1 hunks)src/dbus/mod.rs(1 hunks)src/dbus/pool/mod.rs(1 hunks)src/dbus/pool/pool_3_6/methods.rs(1 hunks)src/dbus/pool/pool_3_6/mod.rs(1 hunks)src/dbus/pool/pool_3_9/mod.rs(1 hunks)src/dbus/pool/shared.rs(1 hunks)src/dbus/types.rs(1 hunks)src/dbus/util.rs(1 hunks)src/engine/engine.rs(1 hunks)src/engine/sim_engine/pool.rs(5 hunks)src/engine/strat_engine/pool/dispatch.rs(1 hunks)src/engine/strat_engine/pool/v1.rs(2 hunks)src/engine/strat_engine/pool/v2.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/dbus/pool/pool_3_6/mod.rs
- src/dbus/filesystem/filesystem_3_0/mod.rs
- src/dbus/filesystem/shared.rs
- src/dbus/util.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
Repo: stratis-storage/stratisd PR: 3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/engine/engine.rssrc/engine/strat_engine/pool/dispatch.rssrc/dbus/pool/shared.rssrc/dbus/manager/manager_3_0/mod.rssrc/dbus/manager/manager_3_0/methods.rssrc/dbus/pool/pool_3_6/methods.rssrc/dbus/pool/pool_3_9/mod.rssrc/dbus/manager/manager_3_9/mod.rssrc/dbus/manager/mod.rssrc/dbus/pool/mod.rssrc/engine/strat_engine/pool/v2.rs
📚 Learning: 2025-09-05T14:32:42.528Z
Learnt from: jbaublitz
Repo: stratis-storage/stratisd PR: 3898
File: src/engine/sim_engine/pool.rs:934-948
Timestamp: 2025-09-05T14:32:42.528Z
Learning: In src/engine/sim_engine/pool.rs, the start_encrypt_pool method intentionally creates a SizedKeyMemory with 1 byte allocated but 0 length reported - this is to avoid NULL pointer issues while simulating an empty key slice in the sim engine.
Applied to files:
src/engine/engine.rssrc/dbus/manager/manager_3_0/methods.rssrc/engine/sim_engine/pool.rs
🧬 Code graph analysis (18)
src/engine/engine.rs (7)
src/engine/sim_engine/pool.rs (1)
create_filesystems(221-263)src/engine/strat_engine/pool/dispatch.rs (1)
create_filesystems(111-121)src/engine/strat_engine/pool/v1.rs (1)
create_filesystems(846-895)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)src/dbus_api/pool/pool_3_0/methods.rs (1)
create_filesystems(29-129)src/dbus_api/pool/pool_3_6/methods.rs (1)
create_filesystems(21-130)src/engine/shared.rs (1)
specs(248-256)
src/dbus/types.rs (1)
src/stratis/keys.rs (1)
engine(28-46)
src/dbus/filesystem/filesystem_3_9/mod.rs (3)
src/dbus/filesystem/filesystem_3_0/props.rs (2)
name_prop(13-15)uuid_prop(9-11)src/dbus/filesystem/shared.rs (1)
filesystem_prop(11-30)src/dbus/pool/pool_3_9/mod.rs (5)
new(38-52)register(54-66)connection(72-72)Uuid(97-99)Name(103-105)
src/engine/strat_engine/pool/dispatch.rs (4)
src/engine/engine.rs (1)
create_filesystems(162-167)src/engine/sim_engine/pool.rs (1)
create_filesystems(221-263)src/engine/strat_engine/pool/v1.rs (1)
create_filesystems(846-895)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)
src/dbus/pool/shared.rs (1)
src/engine/engine.rs (1)
get_pool(487-490)
src/dbus/manager/manager_3_0/mod.rs (2)
src/dbus/manager/manager_3_0/methods.rs (3)
create_pool_method(30-88)destroy_pool_method(90-135)devs(42-42)src/dbus/manager/manager_3_0/props.rs (1)
version_prop(7-9)
src/dbus/manager/manager_3_0/methods.rs (3)
src/dbus/pool/mod.rs (2)
register_pool(29-54)unregister_pool(56-74)src/dbus/util.rs (4)
engine_to_dbus_err_tuple(37-43)tuple_to_option(19-25)connection(54-56)connection(67-69)src/engine/types/keys.rs (1)
new_legacy(103-122)
src/dbus/mod.rs (4)
src/dbus/util.rs (3)
connection(54-56)connection(67-69)send_fs_background_signals(85-91)src/dbus/manager/mod.rs (1)
register_manager(108-127)src/dbus/udev.rs (1)
new(25-39)src/engine/structures/lock.rs (1)
new_shared(84-86)
src/dbus/pool/pool_3_6/methods.rs (1)
src/dbus/filesystem/mod.rs (1)
register_filesystem(25-43)
src/engine/strat_engine/pool/v1.rs (5)
src/engine/engine.rs (3)
create_filesystems(162-167)size(93-93)size(122-122)src/engine/sim_engine/pool.rs (2)
create_filesystems(221-263)new(58-77)src/engine/strat_engine/pool/dispatch.rs (1)
create_filesystems(111-121)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)src/dbus_api/pool/pool_3_0/methods.rs (1)
create_filesystems(29-129)
src/dbus/pool/pool_3_9/mod.rs (3)
src/dbus/pool/pool_3_0/props.rs (5)
allocated_prop(35-37)name_prop(18-21)size_prop(23-25)used_prop(27-33)uuid_prop(14-16)src/dbus/pool/pool_3_6/methods.rs (1)
create_filesystems_method(25-129)src/dbus/pool/shared.rs (2)
pool_prop(21-32)try_pool_prop(34-46)
src/dbus/manager/manager_3_9/mod.rs (2)
src/dbus/manager/manager_3_0/props.rs (1)
version_prop(7-9)src/dbus/manager/manager_3_8/methods.rs (4)
create_pool_method(32-129)devs(47-47)key_desc(48-51)clevis_info(52-52)
src/dbus/filesystem/filesystem_3_0/props.rs (2)
src/dbus/pool/pool_3_9/mod.rs (1)
Name(103-105)src/dbus/filesystem/filesystem_3_9/mod.rs (1)
Name(62-64)
src/dbus/manager/mod.rs (4)
src/dbus/pool/pool_3_9/mod.rs (2)
connection(72-72)register(54-66)src/dbus/util.rs (2)
connection(54-56)connection(67-69)src/dbus/manager/manager_3_0/mod.rs (1)
register(46-58)src/dbus/manager/manager_3_9/mod.rs (1)
register(43-55)
src/dbus/pool/mod.rs (2)
src/dbus/udev.rs (1)
register_pool(62-72)src/dbus/pool/pool_3_9/mod.rs (3)
connection(72-72)register(54-66)unregister(68-74)
src/engine/sim_engine/pool.rs (4)
src/engine/engine.rs (3)
create_filesystems(162-167)size(93-93)size(122-122)src/engine/strat_engine/pool/dispatch.rs (1)
create_filesystems(111-121)src/engine/strat_engine/pool/v1.rs (1)
create_filesystems(846-895)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)
src/dbus/filesystem/mod.rs (2)
src/dbus/pool/pool_3_9/mod.rs (2)
connection(72-72)register(54-66)src/dbus/filesystem/filesystem_3_9/mod.rs (1)
register(38-49)
src/engine/strat_engine/pool/v2.rs (2)
src/engine/shared.rs (1)
specs(248-256)src/engine/types/mod.rs (4)
new(158-160)new(257-259)new(290-302)new(355-357)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (33)
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile stratisd-tools, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile test, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-utils, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-no-ipc, 1.91.0, cargo)
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.91.0, cargo)
- GitHub Check: python-based-tests
- GitHub Check: tests-with-testing-repo (master)
- GitHub Check: stratis-min-cli-checks
- GitHub Check: checks (make -f Makefile test, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile build-min, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile docs-ci, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.91.0, clippy)
- GitHub Check: checks (make -f Makefile build, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-utils, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile stratisd-tools, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build, 1.91.0, cargo)
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.91.0, cargo)
🔇 Additional comments (19)
Cargo.toml (1)
233-236: LGTM! Clean migration to zbus.The dependency migration from
dbus/dbus-treetozbusv5.12.0 is well-executed. Thetokiofeature flag correctly aligns with the async/await architecture introduced in the new D-Bus handlers.Also applies to: 292-292
src/dbus/types.rs (3)
9-19: LGTM! Type trait implementations are correct.The
Typetrait implementations forKeyDescription,PoolUuid, andNamecorrectly useSignature::Str, which is appropriate for these string-based types in the D-Bus protocol.
21-21: LGTM! FilesystemSpec type alias improves clarity.The
FilesystemSpec<'a>type alias provides a clear, reusable definition for filesystem specifications across D-Bus interfaces, representing(name, optional_size, optional_size_limit)tuples.
23-28: LGTM! DbusErrorEnum implementation is appropriate.The
DbusErrorEnumwithOKandERRORvariants provides a simple, clear error code representation for D-Bus responses. The#[allow(non_camel_case_types)]attribute is appropriate for maintaining D-Bus naming conventions.Note: The previous review comment about an empty enum violation is no longer applicable to this implementation.
src/engine/engine.rs (1)
162-167: LGTM! Signature change improves type safety.The updated
create_filesystemssignature removes lifetime complexity and returnsNameinstead of&str, providing better type safety and ownership semantics. This change is consistently implemented across all Pool implementations (sim_engine, strat_engine v1/v2).src/dbus/filesystem/filesystem_3_0/props.rs (1)
9-15: LGTM! Property helpers are correct and follow consistent patterns.Both
uuid_propandname_propprovide straightforward conversions for D-Bus property exposure. The unused parameters inname_propmaintain signature consistency with other property getters that may need access to the filesystem object.src/dbus/filesystem/mod.rs (1)
25-43: LGTM! Filesystem registration follows correct patterns.The
register_filesystemfunction correctly:
- Uses
Ordering::AcqRelfor atomic counter operations- Registers the D-Bus object before updating Manager state
- Propagates errors appropriately with
?src/engine/sim_engine/pool.rs (2)
221-263: LGTM! create_filesystems correctly implements the new signature.The implementation properly:
- Matches the updated trait signature (lines 221-226)
- Wraps filesystem names in
Nametype before returning (line 258)- Maintains the existing validation and creation logic
1137-1137: LGTM! Tests correctly updated for Name type.The test assertions properly use
.as_ref()to convertNameto&strfor string comparisons (lines 1137, 1276, 1330), maintaining test correctness with the new return type.Also applies to: 1276-1276, 1330-1330
src/engine/strat_engine/pool/v1.rs (1)
846-895: LGTM! create_filesystems correctly implements the new signature.The implementation properly:
- Matches the updated trait signature (lines 846-851)
- Wraps filesystem names in
Name::new(name.to_string())before returning (line 890)- Preserves all existing validation, overprovisioning checks, and filesystem creation logic
src/dbus/manager/manager_3_0/methods.rs (1)
66-66: No lock ordering issue exists at this call site.The
create_pool_methoddoes not acquire the manager lock before callingregister_pool. Theregister_poolfunction correctly acquires its own lock (manager.write().await) at the point where it needs to modify manager state. Since there is no nested or recursive locking, no deadlock risk exists.Likely an incorrect or invalid review comment.
src/dbus/pool/shared.rs (1)
21-46: Nice consolidation of pool property access. Centralizing the guard lookup and conversion trims duplication in the property handlers.src/dbus/pool/pool_3_9/mod.rs (1)
77-123: Property handlers look solid. The new interface cleanly reuses the shared helpers and keeps the signal semantics intact.src/dbus/manager/manager_3_9/mod.rs (1)
58-94: Manager v3.9 wiring is clean. Delegating to the shared create_pool method keeps the versioned surface thin and consistent.src/engine/strat_engine/pool/v2.rs (1)
750-800: Result tuple now matches the public Name type. This keeps the engine API in sync with the D-Bus layer without disturbing existing validation.src/dbus/manager/manager_3_0/mod.rs (1)
61-98: R0 interface fits the new pattern. The older manager version now mirrors the zbus-driven setup, which should simplify future maintenance.src/dbus/manager/mod.rs (3)
30-38: LGTM! Clean bidirectional mapping design.The Manager struct provides efficient O(1) lookups in both directions (path→UUID and UUID→path) for pools, filesystems, and block devices.
62-105: LGTM! Consistent query and removal logic.The getter methods provide appropriate access (cloned UUIDs, borrowed paths), and the removal methods correctly maintain bidirectional consistency by removing from both maps.
108-127: LGTM! Proper registration sequence.The function correctly registers both manager interfaces (R0 and R9) at the base path, followed by the ObjectManager. The Arc cloning and ownership transfer are handled appropriately for each registration call.
jbaublitz
force-pushed
the
issue-stratisd-2574
branch
from
c63b1e1 to
a5e181c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/dbus/manager/manager_3_0/methods.rs (1)
102-106: Interpolate the pool path in the error string.The returned message still has the literal
{pool}placeholder. Format in the actual object path so callers see the real value.Apply this diff:
- "Object path {pool} not associated with pool".to_string(), + format!("Object path {} not associated with pool", pool),src/dbus/manager/mod.rs (1)
41-60: Bidirectional map invariant can be violated.The
add_*methods don't handle cases where the UUID or path already exists with a different counterpart, and each method clones the path twice unnecessarily. This issue has been previously identified and detailed solutions have been provided in prior reviews.
🧹 Nitpick comments (1)
src/dbus/pool/pool_3_6/methods.rs (1)
72-80: Avoid cloning the pool name on the hot pathWe already have
&Namefrom the guard, so we can rely on deref coercion to supply the&strwithout allocating a temporaryString.- name.to_string().as_str(), + name,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
Cargo.toml(2 hunks)src/dbus/filesystem/filesystem_3_0/mod.rs(1 hunks)src/dbus/filesystem/filesystem_3_0/props.rs(1 hunks)src/dbus/filesystem/filesystem_3_9/mod.rs(1 hunks)src/dbus/filesystem/mod.rs(1 hunks)src/dbus/filesystem/shared.rs(1 hunks)src/dbus/manager/manager_3_0/methods.rs(1 hunks)src/dbus/manager/manager_3_0/mod.rs(1 hunks)src/dbus/manager/manager_3_8/methods.rs(1 hunks)src/dbus/manager/manager_3_9/mod.rs(1 hunks)src/dbus/manager/mod.rs(1 hunks)src/dbus/mod.rs(1 hunks)src/dbus/pool/mod.rs(1 hunks)src/dbus/pool/pool_3_0/mod.rs(1 hunks)src/dbus/pool/pool_3_0/props.rs(1 hunks)src/dbus/pool/pool_3_6/methods.rs(1 hunks)src/dbus/pool/pool_3_6/mod.rs(1 hunks)src/dbus/pool/pool_3_9/mod.rs(1 hunks)src/dbus/pool/shared.rs(1 hunks)src/dbus/types.rs(1 hunks)src/dbus/udev.rs(1 hunks)src/dbus/util.rs(1 hunks)src/engine/engine.rs(1 hunks)src/engine/mod.rs(1 hunks)src/engine/sim_engine/pool.rs(5 hunks)src/engine/strat_engine/pool/dispatch.rs(1 hunks)src/engine/strat_engine/pool/v1.rs(2 hunks)src/engine/strat_engine/pool/v2.rs(2 hunks)src/stratis/dm.rs(4 hunks)src/stratis/run.rs(2 hunks)src/stratis/timer.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/dbus/filesystem/filesystem_3_0/mod.rs
- src/engine/mod.rs
- src/dbus/udev.rs
- src/dbus/types.rs
- src/dbus/mod.rs
- src/dbus/filesystem/shared.rs
- src/engine/strat_engine/pool/v2.rs
- src/dbus/util.rs
- src/dbus/manager/manager_3_0/mod.rs
- src/dbus/pool/mod.rs
- src/dbus/manager/manager_3_9/mod.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
Repo: stratis-storage/stratisd PR: 3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/dbus/pool/pool_3_0/mod.rssrc/dbus/pool/pool_3_6/mod.rssrc/dbus/manager/manager_3_0/methods.rssrc/dbus/pool/shared.rssrc/stratis/dm.rssrc/dbus/pool/pool_3_6/methods.rssrc/dbus/manager/manager_3_8/methods.rssrc/dbus/pool/pool_3_0/props.rssrc/engine/engine.rssrc/dbus/manager/mod.rssrc/stratis/timer.rssrc/engine/strat_engine/pool/dispatch.rssrc/engine/strat_engine/pool/v1.rssrc/dbus/pool/pool_3_9/mod.rs
📚 Learning: 2025-09-05T14:32:42.528Z
Learnt from: jbaublitz
Repo: stratis-storage/stratisd PR: 3898
File: src/engine/sim_engine/pool.rs:934-948
Timestamp: 2025-09-05T14:32:42.528Z
Learning: In src/engine/sim_engine/pool.rs, the start_encrypt_pool method intentionally creates a SizedKeyMemory with 1 byte allocated but 0 length reported - this is to avoid NULL pointer issues while simulating an empty key slice in the sim engine.
Applied to files:
src/dbus/manager/manager_3_0/methods.rssrc/dbus/manager/manager_3_8/methods.rssrc/engine/engine.rssrc/engine/sim_engine/pool.rs
🧬 Code graph analysis (19)
src/dbus/pool/pool_3_0/mod.rs (1)
src/dbus/pool/pool_3_0/props.rs (5)
allocated_prop(33-35)name_prop(16-19)size_prop(21-23)used_prop(25-31)uuid_prop(12-14)
src/dbus/pool/pool_3_6/mod.rs (1)
src/dbus/pool/pool_3_6/methods.rs (1)
create_filesystems_method(25-129)
src/dbus/manager/manager_3_0/methods.rs (3)
src/dbus/pool/mod.rs (2)
register_pool(29-54)unregister_pool(56-70)src/dbus/util.rs (4)
engine_to_dbus_err_tuple(37-43)tuple_to_option(19-25)connection(54-56)connection(67-69)src/engine/types/keys.rs (1)
new_legacy(103-122)
src/dbus/filesystem/filesystem_3_0/props.rs (2)
src/dbus/pool/pool_3_9/mod.rs (1)
Name(98-100)src/dbus/filesystem/filesystem_3_9/mod.rs (1)
Name(57-59)
src/dbus/pool/shared.rs (1)
src/engine/engine.rs (1)
get_pool(487-490)
src/stratis/dm.rs (1)
src/dbus/util.rs (4)
send_fs_background_signals(85-91)send_pool_background_signals(46-82)connection(54-56)connection(67-69)
src/dbus/pool/pool_3_6/methods.rs (1)
src/dbus/filesystem/mod.rs (1)
register_filesystem(25-43)
src/dbus/manager/manager_3_8/methods.rs (2)
src/dbus/pool/mod.rs (1)
register_pool(29-54)src/dbus/util.rs (4)
engine_to_dbus_err_tuple(37-43)tuple_to_option(19-25)connection(54-56)connection(67-69)
src/dbus/pool/pool_3_0/props.rs (1)
src/dbus/util.rs (1)
option_to_tuple(28-33)
src/engine/engine.rs (7)
src/engine/sim_engine/pool.rs (1)
create_filesystems(221-263)src/engine/strat_engine/pool/dispatch.rs (1)
create_filesystems(111-121)src/engine/strat_engine/pool/v1.rs (1)
create_filesystems(846-895)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)src/dbus_api/pool/pool_3_0/methods.rs (1)
create_filesystems(29-129)src/dbus_api/pool/pool_3_6/methods.rs (1)
create_filesystems(21-130)src/engine/shared.rs (1)
specs(248-256)
src/dbus/filesystem/mod.rs (1)
src/dbus/filesystem/filesystem_3_9/mod.rs (1)
register(33-44)
src/dbus/manager/mod.rs (2)
src/dbus/manager/manager_3_0/mod.rs (1)
register(46-58)src/dbus/manager/manager_3_9/mod.rs (1)
register(43-55)
src/stratis/timer.rs (1)
src/dbus/util.rs (4)
send_fs_background_signals(85-91)send_pool_background_signals(46-82)connection(54-56)connection(67-69)
src/stratis/run.rs (5)
src/dbus/mod.rs (1)
create_dbus_handler(29-56)src/stratis/ipc_support/dbus_support.rs (1)
setup(23-51)src/stratis/ipc_support/jsonrpc_support.rs (1)
setup(39-56)src/stratis/dm.rs (1)
dm_event_thread(36-110)src/stratis/timer.rs (1)
run_timers(74-88)
src/engine/strat_engine/pool/dispatch.rs (6)
src/engine/engine.rs (1)
create_filesystems(162-167)src/engine/sim_engine/pool.rs (1)
create_filesystems(221-263)src/engine/strat_engine/pool/v1.rs (1)
create_filesystems(846-895)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)src/dbus_api/pool/pool_3_0/methods.rs (1)
create_filesystems(29-129)src/engine/shared.rs (1)
specs(248-256)
src/engine/strat_engine/pool/v1.rs (5)
src/engine/engine.rs (3)
create_filesystems(162-167)size(93-93)size(122-122)src/engine/sim_engine/pool.rs (2)
create_filesystems(221-263)new(58-77)src/engine/strat_engine/pool/dispatch.rs (1)
create_filesystems(111-121)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)src/dbus_api/pool/pool_3_0/methods.rs (1)
create_filesystems(29-129)
src/dbus/pool/pool_3_9/mod.rs (3)
src/dbus/pool/pool_3_0/props.rs (5)
allocated_prop(33-35)name_prop(16-19)size_prop(21-23)used_prop(25-31)uuid_prop(12-14)src/dbus/pool/pool_3_6/methods.rs (1)
create_filesystems_method(25-129)src/dbus/pool/shared.rs (2)
pool_prop(21-29)try_pool_prop(31-39)
src/engine/sim_engine/pool.rs (7)
src/engine/engine.rs (3)
create_filesystems(162-167)size(93-93)size(122-122)src/engine/strat_engine/pool/dispatch.rs (1)
create_filesystems(111-121)src/engine/strat_engine/pool/v1.rs (1)
create_filesystems(846-895)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)src/engine/shared.rs (1)
specs(248-256)src/engine/types/actions.rs (3)
new(235-237)new(311-313)new(586-589)src/engine/types/mod.rs (6)
new(158-160)new(257-259)new(290-302)new(355-357)as_ref(168-170)as_ref(392-397)
src/dbus/filesystem/filesystem_3_9/mod.rs (2)
src/dbus/filesystem/filesystem_3_0/props.rs (2)
name_prop(11-13)uuid_prop(7-9)src/dbus/filesystem/shared.rs (1)
filesystem_prop(11-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (32)
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-utils, 1.91.0, cargo)
- GitHub Check: python-based-tests
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-no-ipc, 1.91.0, cargo)
- GitHub Check: stratis-min-cli-checks
- GitHub Check: tests-with-testing-repo (master)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min-no-systemd, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.91.0, clippy)
- GitHub Check: checks-with-ci-repo
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.91.0, cargo)
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
- GitHub Check: checks (make -f Makefile build-min, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile test, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile docs-ci, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.91.0, clippy)
- GitHub Check: checks (make -f Makefile build, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min, 1.91.0, cargo)
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.91.0, cargo)
🔇 Additional comments (23)
src/dbus/pool/pool_3_6/mod.rs (1)
1-7: LGTM!The module structure is clean and follows standard Rust conventions for organizing and re-exporting D-Bus method implementations.
src/dbus/pool/pool_3_0/mod.rs (1)
1-7: LGTM!The module structure is clean and follows standard Rust conventions for organizing and re-exporting pool property helper functions.
Cargo.toml (2)
292-292: LGTM!The feature flag update correctly replaces the old
dbus/dbus-treedependencies withzbus, maintaining the optional D-Bus support pattern.
233-236: ---zbus v5.12.0 is current with no security advisories.
The latest stable zbus crate version is 5.12.0, matching the version specified in the PR. No publicly disclosed security advisories affect zbus v5.12.0. The migration to zbus with the
tokiofeature is appropriate for the async runtime.src/dbus/filesystem/filesystem_3_0/props.rs (1)
7-13: LGTM!The property helper functions are straightforward string conversions that align with the D-Bus filesystem interface requirements.
src/engine/engine.rs (1)
162-167: LGTM!The signature update removes the lifetime parameter and returns
Nameinstead of&str, improving type safety and simplifying the API. This aligns with the broader migration to use theNametype for filesystem identifiers across the codebase.src/dbus/filesystem/mod.rs (1)
25-43: LGTM!The
register_filesystemfunction follows a clean pattern: generates a unique path, registers with D-Bus, updates the manager, and returns the path. Error handling is appropriate.src/engine/sim_engine/pool.rs (3)
221-226: LGTM!The signature update matches the trait definition, removing the lifetime parameter and returning
Nameinstead of&str.
258-258: LGTM!Correctly wraps the filesystem name with
Name::new()to match the updated return type.
1137-1137: LGTM!Test assertions correctly updated to use
.as_ref()for name comparisons, since the return type now usesNameinstead of&str.Also applies to: 1276-1276, 1330-1330
src/stratis/run.rs (4)
25-25: LGTM!The import path update reflects the migration from the old
dbus_apimodule to the newdbusmodule with zbus-based implementation.
100-100: LGTM!The replacement of channel-based D-Bus communication with
create_dbus_handlersimplifies the IPC setup and aligns with the zbus migration. The function returns the necessary handles (connection,udev,manager) for D-Bus operations.
103-112: LGTM!The conditional IPC setup correctly branches based on the
dbus_enabledfeature flag, passing the appropriate parameters to each path.
115-130: LGTM!The D-Bus connection and manager are now correctly passed to
dm_event_threadandrun_timerswhen D-Bus is enabled, allowing these components to send background signals.src/dbus/pool/shared.rs (1)
11-19: Centralizing the guard lookup looks solidThe shared helper keeps the property accessors consistent and gives a single place to format the “pool missing” failure.
src/engine/strat_engine/pool/v1.rs (1)
845-895: Glad to see theNametype propagatedReturning
Namealongside the UUID keeps the engine-side results aligned with the new D-Bus registration flow.src/stratis/dm.rs (1)
65-74: Nice tie-in with the async signal helpersDriving the background D-Bus updates directly from the DM loop keeps the new zbus layer in sync without the sender indirection.
src/dbus/pool/pool_3_0/props.rs (1)
21-35: Property helpers mirror the on-wire contractThe conversion logic cleanly packages the pool metrics for D-Bus consumers, especially the boolean/tuple handling for
used.src/stratis/timer.rs (3)
7-18: LGTM! Feature gating is consistent.The imports are properly feature-gated for the
dbus_enabledconfiguration, ensuring clean separation of concerns.
74-88: LGTM! Parameter threading is correct.The function signatures are properly updated to thread the
connectionandmanagerparameters through the call chain, with consistent feature gating.
46-48: Verify intentional stub or incomplete implementation of filesystem background signals.The
send_fs_background_signalsfunction insrc/dbus/util.rs:85-90is a stub that acceptsfs_diffsbut immediately returnsOk(())without processing them. While the parallelsend_pool_background_signalsimplementation (lines 46-82) actively emits D-Bus signals, filesystem signals are never emitted. The FilesystemR9 D-Bus interface (src/dbus/filesystem/filesystem_3_9/mod.rs) does define properties withemits_changed_signalcapability, but this stub doesn't utilize them. No TODO or documentation explains why filesystem signals are unimplemented. Confirm whether this is an intentional design decision (and if so, document it) or incomplete work requiring completion.src/dbus/manager/mod.rs (2)
86-105: LGTM! Remove methods correctly maintain the bidirectional invariant.Each
remove_*method properly removes both the path→UUID and UUID→path mappings, ensuring consistency.
108-127: LGTM! Registration flow is correct.The function properly registers both manager interface versions (R0 and R9) and the ObjectManager, with appropriate cloning of shared resources.
jbaublitz
force-pushed
the
issue-stratisd-2574
branch
from
a5e181c to
f5d0dd1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/stratis/ipc_support/dbus_support.rs (1)
24-31: Drop the pool guard before awaiting registration.This is the same deadlock risk flagged in the previous review.
engine.pools().awaitreturns anAllLockReadGuard, and the code holds that guard while awaitingudev.register_poolinside the loop. Because register_pool uses async zbus I/O and may need to call back into the engine, holding the guard across the await can block other tasks and cause deadlock.Collect the pool UUIDs first, drop the guard, then await the registrations:
- { - let pools = engine.pools().await; - for (_, pool_uuid, _) in pools.iter() { - if let Err(e) = udev.register_pool(*pool_uuid).await { - warn!("Failed to register existing pools: {e}"); - } - } - } + let pool_uuids = { + let pools = engine.pools().await; + pools + .iter() + .map(|(_, pool_uuid, _)| *pool_uuid) + .collect::<Vec<_>>() + }; + + for pool_uuid in pool_uuids { + if let Err(e) = udev.register_pool(pool_uuid).await { + warn!("Failed to register existing pools: {e}"); + } + }src/dbus/manager/mod.rs (1)
41-60: Bidirectional map invariant can be violated.The
add_*methods don't handle cases where the UUID or path already exists with a different counterpart, which can break the bidirectional invariant. Additionally,path.clone()is called twice per insertion—an owned clone could be shared.Scenario:
add_pool(path1, uuid1) // path1→uuid1, uuid1→path1 ✓ add_pool(path2, uuid1) // path2→uuid1, uuid1→path2 // Now: path1→uuid1 (stale), path2→uuid1, uuid1→path2 // Invariant broken: path1 maps to uuid1, but uuid1 doesn't map back to path1Apply this diff to
add_poolto remove stale entries and eliminate double clone:pub fn add_pool(&mut self, path: &ObjectPath<'_>, uuid: PoolUuid) { - self.pool_path_to_uuid - .insert(OwnedObjectPath::from(path.clone()), uuid); - self.pool_uuid_to_path - .insert(uuid, OwnedObjectPath::from(path.clone())); + let owned_path = OwnedObjectPath::from(path.clone()); + + // Remove stale reverse mapping if UUID was previously mapped to a different path + if let Some(old_path) = self.pool_uuid_to_path.get(&uuid) { + if old_path != &owned_path { + self.pool_path_to_uuid.remove(old_path); + } + } + + // Remove stale forward mapping if path was previously mapped to a different UUID + if let Some(old_uuid) = self.pool_path_to_uuid.get(&owned_path) { + if old_uuid != &uuid { + self.pool_uuid_to_path.remove(old_uuid); + } + } + + self.pool_path_to_uuid.insert(owned_path.clone(), uuid); + self.pool_uuid_to_path.insert(uuid, owned_path); }Apply similar fixes to
add_filesystemandadd_blockdev.src/dbus/manager/manager_3_0/methods.rs (2)
173-173: Fix string interpolation bug in error message.Line 173 contains a string interpolation error. The error message references
{pool}, but it should interpolate the object path value.Apply this diff:
- "Object path {pool} not associated with pool".to_string(), + format!("Object path {} not associated with pool", pool),
166-203: Release the manager lock before awaiting destroy/unregister.The write guard is held while awaiting
engine.destroy_poolandunregister_pool. Sinceunregister_poolcallsmanager.write().await, this causes a deadlock.Apply this diff to scope the lock:
- let manager_lock = manager.write().await; - let uuid = match manager_lock.pool_get_uuid(&pool) { - Some(u) => u, - None => { - return ( - default_return, - DbusErrorEnum::ERROR as u16, - "Object path {pool} not associated with pool".to_string(), - ); - } - }; + let uuid = { + let manager_guard = manager.write().await; + match manager_guard.pool_get_uuid(&pool) { + Some(u) => u, + None => { + return ( + default_return, + DbusErrorEnum::ERROR as u16, + format!("Object path {} not associated with pool", pool), + ); + } + } + };
🧹 Nitpick comments (2)
src/dbus/macros.rs (1)
5-13: Consider adding documentation.The macro would benefit from a doc comment explaining its purpose, expected usage, and the
Displaytrait requirement for theOkvariant. Additionally, consider whetherErrvariants should also be logged for debugging purposes.Apply this diff to add documentation:
+/// Logs and returns the result of a D-Bus action. +/// +/// If the action succeeds (Ok variant), logs the value using `log::info!`. +/// The Ok value must implement the `Display` trait. +/// +/// # Example +/// ```ignore +/// let result = handle_action!(some_dbus_operation()); +/// ``` macro_rules! handle_action { ($action:expr) => {{ let action = $action; if let Ok(ref a) = action { log::info!("{}", a); } action }}; }src/dbus/pool/shared.rs (1)
21-39: Clarify the distinction betweenpool_propandtry_pool_prop.Both functions have identical implementations—they call
get_pooland apply the closurefto the guard. Thetry_prefix typically suggests different error handling or fallible operations, but there's no functional difference here.Is this duplication intentional for future differentiation, or can these be consolidated into a single function?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
Cargo.toml(2 hunks)src/dbus/consts.rs(1 hunks)src/dbus/filesystem/filesystem_3_0/mod.rs(1 hunks)src/dbus/filesystem/filesystem_3_0/props.rs(1 hunks)src/dbus/filesystem/filesystem_3_9/mod.rs(1 hunks)src/dbus/filesystem/mod.rs(1 hunks)src/dbus/filesystem/shared.rs(1 hunks)src/dbus/macros.rs(1 hunks)src/dbus/manager/manager_3_0/methods.rs(1 hunks)src/dbus/manager/manager_3_0/mod.rs(1 hunks)src/dbus/manager/manager_3_0/props.rs(1 hunks)src/dbus/manager/manager_3_8/methods.rs(1 hunks)src/dbus/manager/manager_3_8/mod.rs(1 hunks)src/dbus/manager/manager_3_9/mod.rs(1 hunks)src/dbus/manager/mod.rs(1 hunks)src/dbus/mod.rs(1 hunks)src/dbus/pool/mod.rs(1 hunks)src/dbus/pool/pool_3_0/mod.rs(1 hunks)src/dbus/pool/pool_3_0/props.rs(1 hunks)src/dbus/pool/pool_3_6/methods.rs(1 hunks)src/dbus/pool/pool_3_6/mod.rs(1 hunks)src/dbus/pool/pool_3_9/mod.rs(1 hunks)src/dbus/pool/shared.rs(1 hunks)src/dbus/types.rs(1 hunks)src/dbus/udev.rs(1 hunks)src/dbus/util.rs(1 hunks)src/engine/engine.rs(1 hunks)src/engine/mod.rs(1 hunks)src/engine/sim_engine/pool.rs(5 hunks)src/engine/strat_engine/pool/dispatch.rs(1 hunks)src/engine/strat_engine/pool/v1.rs(2 hunks)src/engine/strat_engine/pool/v2.rs(2 hunks)src/engine/types/actions.rs(2 hunks)src/engine/types/keys.rs(3 hunks)src/engine/types/mod.rs(4 hunks)src/stratis/dm.rs(4 hunks)src/stratis/errors.rs(3 hunks)src/stratis/ipc_support/dbus_support.rs(1 hunks)src/stratis/ipc_support/jsonrpc_support.rs(2 hunks)src/stratis/run.rs(2 hunks)src/stratis/timer.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/dbus/pool/pool_3_6/mod.rs
- src/dbus/consts.rs
- src/dbus/manager/manager_3_8/mod.rs
- src/dbus/udev.rs
- src/engine/strat_engine/pool/dispatch.rs
- src/dbus/filesystem/mod.rs
- src/stratis/ipc_support/jsonrpc_support.rs
- src/engine/mod.rs
- src/dbus/filesystem/filesystem_3_0/props.rs
- src/dbus/pool/mod.rs
- src/dbus/pool/pool_3_6/methods.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
Repo: stratis-storage/stratisd PR: 3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/dbus/pool/pool_3_0/mod.rssrc/stratis/ipc_support/dbus_support.rssrc/engine/strat_engine/pool/v2.rssrc/stratis/dm.rssrc/dbus/util.rssrc/dbus/manager/manager_3_8/methods.rssrc/stratis/timer.rssrc/engine/engine.rssrc/dbus/manager/manager_3_0/methods.rssrc/dbus/pool/pool_3_9/mod.rssrc/engine/strat_engine/pool/v1.rssrc/dbus/manager/manager_3_9/mod.rssrc/dbus/pool/pool_3_0/props.rssrc/dbus/manager/mod.rssrc/dbus/pool/shared.rssrc/dbus/manager/manager_3_0/mod.rs
📚 Learning: 2025-09-05T14:32:42.528Z
Learnt from: jbaublitz
Repo: stratis-storage/stratisd PR: 3898
File: src/engine/sim_engine/pool.rs:934-948
Timestamp: 2025-09-05T14:32:42.528Z
Learning: In src/engine/sim_engine/pool.rs, the start_encrypt_pool method intentionally creates a SizedKeyMemory with 1 byte allocated but 0 length reported - this is to avoid NULL pointer issues while simulating an empty key slice in the sim engine.
Applied to files:
src/dbus/manager/manager_3_8/methods.rssrc/engine/sim_engine/pool.rs
🧬 Code graph analysis (26)
src/dbus/pool/pool_3_0/mod.rs (1)
src/dbus/pool/pool_3_0/props.rs (4)
allocated_prop(29-31)name_prop(12-15)size_prop(17-19)used_prop(21-27)
src/stratis/ipc_support/dbus_support.rs (3)
src/stratis/ipc_support/jsonrpc_support.rs (1)
setup(39-56)src/engine/engine.rs (1)
pools(507-507)src/engine/strat_engine/engine.rs (2)
pools(201-203)pools(739-741)
src/engine/strat_engine/pool/v2.rs (2)
src/engine/shared.rs (1)
specs(248-256)src/engine/types/mod.rs (4)
new(178-180)new(277-279)new(310-322)new(375-377)
src/engine/types/actions.rs (2)
src/dbus/pool/pool_3_9/mod.rs (1)
Name(98-100)src/dbus/filesystem/filesystem_3_9/mod.rs (1)
Name(54-56)
src/stratis/dm.rs (2)
src/dbus/util.rs (4)
send_fs_background_signals(85-91)send_pool_background_signals(46-82)connection(54-56)connection(67-69)src/dbus/pool/pool_3_9/mod.rs (1)
connection(67-67)
src/dbus/util.rs (1)
src/dbus/pool/pool_3_9/mod.rs (1)
connection(67-67)
src/dbus/manager/manager_3_8/methods.rs (3)
src/dbus/pool/mod.rs (1)
register_pool(29-54)src/dbus/util.rs (4)
engine_to_dbus_err_tuple(37-43)tuple_to_option(19-25)connection(54-56)connection(67-69)src/engine/types/keys.rs (12)
new(46-48)new(126-144)new(278-282)new(728-730)from(251-258)from(645-661)from(669-677)from(799-806)try_from(499-521)try_from(705-713)try_from(719-721)uuid(732-734)
src/stratis/timer.rs (1)
src/dbus/util.rs (4)
send_fs_background_signals(85-91)send_pool_background_signals(46-82)connection(54-56)connection(67-69)
src/engine/engine.rs (6)
src/engine/sim_engine/pool.rs (1)
create_filesystems(221-263)src/engine/strat_engine/pool/dispatch.rs (1)
create_filesystems(111-121)src/engine/strat_engine/pool/v1.rs (1)
create_filesystems(846-895)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)src/dbus_api/pool/pool_3_6/methods.rs (1)
create_filesystems(21-130)src/engine/shared.rs (1)
specs(248-256)
src/engine/sim_engine/pool.rs (5)
src/engine/engine.rs (3)
create_filesystems(162-167)size(93-93)size(122-122)src/engine/strat_engine/pool/v1.rs (1)
create_filesystems(846-895)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)src/engine/types/actions.rs (3)
new(235-237)new(311-313)new(586-589)src/engine/types/mod.rs (6)
new(178-180)new(277-279)new(310-322)new(375-377)as_ref(188-190)as_ref(412-417)
src/dbus/macros.rs (1)
src/bin/utils/generators/lib.rs (1)
log(26-28)
src/dbus/filesystem/filesystem_3_9/mod.rs (2)
src/dbus/filesystem/filesystem_3_0/props.rs (1)
name_prop(7-9)src/dbus/filesystem/shared.rs (1)
filesystem_prop(11-27)
src/stratis/run.rs (5)
src/dbus/mod.rs (1)
create_dbus_handler(32-59)src/stratis/ipc_support/dbus_support.rs (1)
setup(23-51)src/stratis/ipc_support/jsonrpc_support.rs (1)
setup(39-56)src/stratis/dm.rs (1)
dm_event_thread(36-110)src/stratis/timer.rs (1)
run_timers(74-88)
src/dbus/filesystem/shared.rs (2)
src/dbus/pool/pool_3_9/mod.rs (1)
Name(98-100)src/dbus/filesystem/filesystem_3_9/mod.rs (1)
Name(54-56)
src/dbus/manager/manager_3_0/methods.rs (3)
src/dbus/pool/mod.rs (2)
register_pool(29-54)unregister_pool(56-70)src/dbus/util.rs (4)
engine_to_dbus_err_tuple(37-43)tuple_to_option(19-25)connection(54-56)connection(67-69)src/engine/types/keys.rs (1)
new_legacy(105-124)
src/dbus/filesystem/filesystem_3_0/mod.rs (1)
src/dbus/filesystem/filesystem_3_0/props.rs (1)
name_prop(7-9)
src/dbus/pool/pool_3_9/mod.rs (3)
src/dbus/pool/pool_3_0/props.rs (4)
allocated_prop(29-31)name_prop(12-15)size_prop(17-19)used_prop(21-27)src/dbus/pool/pool_3_6/methods.rs (1)
create_filesystems_method(25-129)src/dbus/pool/shared.rs (2)
pool_prop(21-29)try_pool_prop(31-39)
src/stratis/errors.rs (2)
src/dbus_api/blockdev/blockdev_3_0/api.rs (9)
f(34-34)f(41-41)f(48-48)f(57-57)f(64-64)f(71-71)f(78-78)f(85-85)f(92-92)src/dbus_api/filesystem/filesystem_3_0/api.rs (3)
f(33-33)f(40-40)f(47-47)
src/dbus/mod.rs (3)
src/dbus/manager/mod.rs (1)
register_manager(108-127)src/dbus/udev.rs (1)
new(25-39)src/engine/structures/lock.rs (1)
new_shared(84-86)
src/engine/strat_engine/pool/v1.rs (5)
src/engine/engine.rs (3)
create_filesystems(162-167)size(93-93)size(122-122)src/engine/sim_engine/pool.rs (2)
create_filesystems(221-263)new(58-77)src/engine/strat_engine/pool/dispatch.rs (1)
create_filesystems(111-121)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)src/dbus_api/pool/pool_3_0/methods.rs (1)
create_filesystems(29-129)
src/dbus/manager/manager_3_9/mod.rs (2)
src/dbus/manager/manager_3_0/props.rs (1)
version_prop(7-9)src/dbus/manager/manager_3_8/methods.rs (4)
create_pool_method(32-130)devs(47-47)key_desc(48-51)clevis_info(52-52)
src/dbus/pool/pool_3_0/props.rs (2)
src/dbus/util.rs (1)
option_to_tuple(28-33)src/dbus/pool/pool_3_9/mod.rs (1)
Name(98-100)
src/engine/types/mod.rs (2)
src/engine/types/keys.rs (1)
uuid(732-734)src/dbus/pool/pool_3_9/mod.rs (2)
Uuid(92-94)Name(98-100)
src/dbus/manager/mod.rs (3)
src/dbus/pool/pool_3_9/mod.rs (2)
connection(67-67)register(49-61)src/dbus/manager/manager_3_0/mod.rs (1)
register(52-64)src/dbus/manager/manager_3_9/mod.rs (1)
register(43-55)
src/dbus/pool/shared.rs (1)
src/engine/engine.rs (1)
get_pool(487-490)
src/dbus/manager/manager_3_0/mod.rs (2)
src/dbus/manager/manager_3_0/methods.rs (6)
create_pool_method(98-156)destroy_pool_method(158-203)list_keys_method(33-43)set_key_method(45-73)unset_key_method(75-95)devs(110-110)src/dbus/manager/manager_3_0/props.rs (1)
version_prop(7-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (33)
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile build, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile test, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile docs-ci, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-no-ipc, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-utils, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile test, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile build, 1.91.0, cargo)
- GitHub Check: python-based-tests
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.91.0, clippy)
- GitHub Check: checks (make -f Makefile build-min, 1.91.0, cargo)
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build, 1.91.0, cargo)
- GitHub Check: tests-with-testing-repo (master)
- GitHub Check: stratis-min-cli-checks
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.91.0, cargo)
🔇 Additional comments (55)
src/dbus/macros.rs (2)
1-4: LGTM!The license header is correctly formatted and consistent with the project's licensing requirements.
5-13: Remove this review comment - the macro export mechanism is correct.The review comment incorrectly claims the macro "lacks
#[macro_export]or explicit visibility." In fact, the macro is properly exported via#[macro_use] mod macros;in dbus/mod.rs, which is the correct Rust idiom for making macros visible throughout a crate's sibling modules. The#[macro_export]attribute is used for external crate visibility, not internal module visibility.Additionally, the macro's usage across the codebase confirms the types passed to
handle_action!implement theDisplaytrait (required forlog::info!("{}", a)), as evidenced by the code compiling successfully.Likely an incorrect or invalid review comment.
src/engine/types/actions.rs (2)
17-17: LGTM! Name type added to public exports.The addition of
Nameto the public import path enables the type strengthening inSetCreateActionand aligns with the broader API transition from borrowed string slices to ownedNametypes.
336-355: LGTM! Type strengthening from&strtoName.The Display implementation for
SetCreateAction<(Name, FilesystemUuid, Sectors)>correctly formats filesystem names using theNametype instead of borrowed string slices. This change aligns with the broader API evolution described in the AI summary.src/dbus/types.rs (2)
5-5: FilesystemSpec type alias is clear and well-defined.The type alias
FilesystemSpec<'a> = Vec<(&'a str, (bool, &'a str), (bool, &'a str))>effectively represents filesystem specifications with optional size and size limit parameters using the tuple-as-option pattern.
7-12: DbusErrorEnum is correctly defined.The enum provides error codes for D-Bus error mapping. The
OK = 0andERROR = 1values are appropriate for status reporting.Cargo.toml (2)
233-236: LGTM! zbus dependency added for D-Bus migration.The addition of
zbusversion 5.12.0 withtokiofeatures aligns with the PR objectives to migrate from the olderdbus/dbus-treelibraries to the modern async-capablezbuslibrary. This version provides good async support and integrates well with the tokio runtime.
292-292: LGTM! Feature gate simplified to use zbus.The
dbus_enabledfeature correctly updated to depend onzbusinstead of the removeddbusanddbus-treedependencies, simplifying the feature definition.src/dbus/pool/shared.rs (1)
11-19: LGTM! Pool lookup helper is correct.The
get_poolfunction properly retrieves a pool by UUID and maps theNonecase to a descriptive D-Bus error.src/dbus/manager/manager_3_8/methods.rs (4)
31-44: LGTM! Function signature appropriately structured.The
#[allow(clippy::too_many_arguments)]attribute is justified here as this function wraps a D-Bus method with multiple required parameters. The signature accurately reflects the D-Bus API surface.
52-70: LGTM! Clevis info parsing with proper error handling.The JSON parsing for Clevis configuration uses
try_foldto collect results and properly maps parsing errors throughengine_to_dbus_err_tuple. Early return on error withdefault_returnis correct.
79-90: LGTM! IntegrityTagSpec validation with clear error messages.The parsing of the integrity tag specification properly uses
IntegrityTagSpec::try_fromand wraps any parsing errors in a descriptive message before mapping to D-Bus error format.
107-118: LGTM! Pool registration with proper error handling.The pool registration flow on successful creation correctly handles both success and failure cases. Registration failures are properly mapped to D-Bus errors while maintaining the appropriate return structure.
src/stratis/errors.rs (3)
53-56: LGTM! Error variants updated for zbus migration.The
Dbusvariant correctly updated to usezbus::Error(withoutArcwrapping), and the newDbusTypevariant added forzbus::zvariant::Errorto handle D-Bus type conversion errors. Both properly gated with#[cfg(feature = "dbus_enabled")].
149-156: LGTM! Display implementations updated for zbus errors.The Display formatting for both
DbusandDbusTypeerror variants provides clear, descriptive error messages consistent with the other error variants in the enum.
230-242: LGTM! From implementations for zbus error types.The
Fromtrait implementations correctly mapzbus::ErrortoStratisError::Dbusandzbus::zvariant::ErrortoStratisError::DbusType, properly gated behind thedbus_enabledfeature.src/dbus/util.rs (4)
18-33: LGTM! D-Bus option conversion utilities.The
tuple_to_optionandoption_to_tuplefunctions correctly convert between the D-Bus tuple-as-option pattern(bool, T)and Rust's nativeOption<T>. These utilities simplify D-Bus API parameter handling throughout the codebase.
35-43: LGTM! Error translation utility with DM error special case.The
engine_to_dbus_err_tuplefunction properly mapsStratisErrorto D-Bus error format(u16, String), with appropriate special handling forDmError::Coreto extract the underlying error message.
45-82: LGTM! Pool background signals correctly implemented.The function properly iterates over pool diffs, checks for changes in
allocated_sizeandtotal_physical_used, retrieves the interface reference, and emits the appropriate property change signals. Error handling correctly returns an error if a pool has no associated D-Bus path.
84-91: Verify if filesystem background signals stub is intentional.The
send_fs_background_signalsfunction is currently a stub that returnsOk(())without emitting any signals. While past review comments indicated stubs were addressed, this one remains.Is this intentional for a phased implementation, or should filesystem property change signals be implemented similarly to pool signals?
src/dbus/pool/pool_3_9/mod.rs (4)
24-47: LGTM! PoolR9 struct and constructor well-structured.The
PoolR9struct correctly holds all necessary state (connection, engine, manager, counter, uuid) for the D-Bus pool interface. The constructor properly initializes all fields.
49-69: LGTM! Registration lifecycle methods correctly implemented.The
registerandunregistermethods properly manage the D-Bus object lifecycle using zbus's object server API. The async interface and error propagation are appropriate.
72-88: LGTM! CreateFilesystems method properly delegates.The method correctly forwards to the shared
create_filesystems_methodimplementation with all required parameters. The#[allow(non_snake_case)]attribute is appropriate for D-Bus naming conventions.
90-118: LGTM! Property implementations with correct signal configuration.The properties are properly configured:
UuidandNameuseemits_changed_signal = "const"since they're immutableTotalPhysicalSize,TotalPhysicalUsed, andAllocatedSizeuse"true"since they can change over timeProperty implementations correctly delegate to
pool_propandtry_pool_prophelpers for safe pool access.src/engine/types/keys.rs (3)
18-19: Well-structured conditional import.The feature gating for zbus dependencies is appropriate and ensures clean builds when D-Bus support is disabled.
470-494: Correct migration to serde_json::Value.The implementation now uses
serde_json::Valueinstead of the dbus-treeValuetype, which aligns with the zbus migration objectives. The JSON structure construction is correct.
681-689: Excellent feature-gated type definition.The conditional derives for
KeyDescriptionare well-structured. Whendbus_enabled, the type includes zvariant derives with the correct signature annotation for D-Bus string serialization. This allows the same type to work seamlessly in both D-Bus and non-D-Bus contexts.src/dbus/pool/pool_3_0/mod.rs (1)
1-7: Clean module organization.The module structure with props submodule and selective re-exports follows good Rust practices. The exposed property helpers (
allocated_prop,name_prop,size_prop,used_prop) align with the D-Bus interface requirements.src/dbus/filesystem/filesystem_3_0/mod.rs (1)
1-7: LGTM!Consistent module organization matching the pool_3_0 pattern. The single export of
name_propis appropriate for the filesystem interface.src/dbus/manager/manager_3_0/props.rs (1)
7-9: Simple and correct property implementation.The
version_propfunction correctly returns the static VERSION string for D-Bus property access.src/engine/engine.rs (1)
162-167: API signature improvement with clearer ownership semantics.The changes to
create_filesystemsremove the lifetime parameter and returnNameinstead of&str. This is a breaking change but provides better ownership semantics—callers now receive owned filesystem names rather than borrowed strings, which is more appropriate for D-Bus interfaces and async contexts where data needs to outlive the original call frame.The removal of the lifetime parameter from input specs simplifies the API without compromising functionality.
src/dbus/filesystem/shared.rs (1)
11-27: Well-designed async property helper.The
filesystem_propfunction follows the established pattern from the pool utilities and provides a clean abstraction for D-Bus property access. The error handling is appropriate, and the callback approach allows flexibility in what property value is extracted without multiple helper functions.src/engine/sim_engine/pool.rs (3)
221-226: Signature correctly updated to match trait.The
create_filesystemsimplementation signature now matches the updatedPooltrait definition, removing lifetime parameters and returningNameinstances.
258-258: Proper Name wrapping for owned return values.The code correctly wraps the filesystem name in a
Nameinstance when building the result tuple. Theto_owned()is necessary to create an ownedStringfrom the borrowed&str.
1137-1137: Tests updated correctly for Name type.The test assertions now properly access the inner string via
x.0.as_ref()to compare with string literals, correctly handling theNamewrapper in the result tuples.Also applies to: 1276-1276, 1330-1330
src/stratis/run.rs (3)
100-100: Correct async initialization of D-Bus handler.The call to
create_dbus_handlerproperly awaits the async operation and destructures the result intoconnection,udev, andmanagercomponents for use in subsequent thread initialization.
103-112: Well-structured conditional IPC setup.The feature-gated initialization correctly:
- Passes the
udevhandler object directly whendbus_enabled(consuming it for async event processing)- Falls back to the
udev_receiverchannel approach when D-Bus is disabledThis maintains compatibility across different build configurations.
114-131: Proper thread wiring with shared connection and manager.The D-Bus connection and manager are correctly passed to both
dm_event_threadandrun_timers:
Arc::clone(&connection)creates a cheap reference-counted clone for thread sharingmanager.clone()is inexpensive as it's aLockable<Arc<RwLock<Manager>>>- The conditional compilation ensures these are only passed when
dbus_enabledsrc/dbus/mod.rs (1)
32-58: Nice consolidation of D-Bus setup.Centralizing the connection build, manager registration, and udev handler construction into one async entry point makes the IPC bootstrap path much clearer and keeps the shared state plumbed consistently.
src/dbus/pool/pool_3_0/props.rs (1)
21-27: Option-to-tuple mapping fits the D-Bus contract.Returning the
(bool, value)pair with a zero default keeps the legacy presence semantics intact while letting callers forward the string payload without extra conversions.src/engine/types/mod.rs (1)
58-105: Feature-gated zvariant support is well-scoped.Deriving
Type/Valueonly underdbus_enabledand providing the string-basedValueconversion keeps UUID wrappers andNameready for zbus without burdening non-DBus builds.src/stratis/dm.rs (1)
65-95: Background signal hook looks solid.Threading the shared manager/connection into the DM event loop and fanning out diffs via the new helpers keeps D-Bus updates local to where engine changes are detected while still guarding failures with warnings.
src/engine/strat_engine/pool/v1.rs (1)
845-895: Returning ownedNameobjects is a clean upgrade.Switching to owned
Namevalues in the creation results avoids lifetime juggling and lines up the V1 pool implementation with the engine trait expectations.src/dbus/filesystem/filesystem_3_9/mod.rs (1)
1-57: LGTM! Clean r9 filesystem interface implementation.The new FilesystemR9 D-Bus object follows established patterns, correctly delegates property resolution to shared helpers, and properly registers with zbus.
src/engine/strat_engine/pool/v2.rs (1)
751-756: LGTM! Consistent migration to Name type.The signature change from
&strtoNamein theSetCreateActionreturn type aligns with the broader migration to use proper Name types throughout the codebase. The wrapping at line 795 correctly constructsNamefrom the string.Also applies to: 795-795
src/stratis/ipc_support/dbus_support.rs (1)
34-50: Clean udev event loop structure.The single-task udev event processing loop with appropriate error handling and logging is well-structured for the new async architecture.
src/stratis/timer.rs (1)
22-88: LGTM! Clean refactor to direct D-Bus signaling.The migration from channel-based D-Bus actions to direct signal emission via
send_pool_background_signalsandsend_fs_background_signalsis well-structured. The conditional compilation, parameter threading, and error handling are all correct.src/dbus/manager/manager_3_9/mod.rs (1)
1-94: LGTM! Standard ManagerR9 implementation.The new ManagerR9 D-Bus interface follows the established pattern of other manager versions, correctly delegates
CreatePoolto the sharedcreate_pool_method, and properly exposes the Version property. The structure and registration flow are consistent with the codebase conventions.src/dbus/manager/mod.rs (2)
86-106: LGTM!The
remove_*methods correctly maintain the bidirectional invariant by removing entries from both maps.
108-127: LGTM!The registration flow correctly registers both ManagerR0 and ManagerR9 interfaces, then binds the ObjectManager to the base path.
src/dbus/manager/manager_3_0/mod.rs (2)
30-64: LGTM!The struct definition and registration pattern follow the standard async D-Bus interface wiring approach used elsewhere in the codebase.
67-123: LGTM!The D-Bus interface implementation correctly delegates to helper functions and follows zbus conventions for method names and property signals.
src/dbus/manager/manager_3_0/methods.rs (3)
33-43: LGTM!The method correctly delegates to the engine's key handler and properly converts errors to D-Bus format.
45-95: LGTM!Both key management methods correctly handle the different action variants (Created, ValueChanged, Deleted, Identity) and properly convert engine errors.
97-156: LGTM!The pool creation flow correctly handles optional encryption parameters, parses Clevis configuration, and registers the pool upon successful creation.
jbaublitz
force-pushed
the
issue-stratisd-2574
branch
2 times, most recently
from
84663c9 to
6f88a9c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
src/dbus/pool/mod.rs (1)
29-54: Critical: Return OwnedObjectPath to fix lifetime safety.Lines 36-40 construct an
ObjectPathfrom a temporaryString(fromformat!()), but the function returnsObjectPath<'a>which is a borrowed type. The temporary String is dropped, making the returned path invalid. Callers will receive unsound references.Apply this fix:
-pub async fn register_pool<'a>( +pub async fn register_pool( engine: &Arc<dyn Engine>, connection: &Arc<Connection>, manager: &Lockable<Arc<RwLock<Manager>>>, counter: &Arc<AtomicU64>, pool_uuid: PoolUuid, -) -> StratisResult<(ObjectPath<'a>, Vec<ObjectPath<'a>>)> { - let path = ObjectPath::try_from(format!( +) -> StratisResult<(OwnedObjectPath, Vec<OwnedObjectPath>)> { + let path_str = format!( "{}/{}", consts::STRATIS_BASE_PATH, counter.fetch_add(1, Ordering::AcqRel), - ))?; + ); + let path = ObjectPath::try_from(path_str.as_str())?; PoolR9::register( engine, connection, manager, counter, - path.clone(), + path, pool_uuid, ) .await?; manager.write().await.add_pool(&path, pool_uuid); - Ok((path, Vec::default())) + Ok((OwnedObjectPath::try_from(path_str)?, Vec::default())) }Also update callers (e.g., src/dbus/manager/manager_3_8/methods.rs) to accept OwnedObjectPath.
src/stratis/ipc_support/dbus_support.rs (1)
23-50: Avoid holding engine pool guard across async D‑Bus registration
engine.pools().awaitreturns a guard over all pools, and the current code keeps that guard alive while awaitingudev.register_pool(*pool_uuid).awaitinside the loop. That can block other tasks needing pool access and risks deadlock if registration paths call back into the engine.A safer pattern is to collect the UUIDs while the guard is held, drop the guard, and then perform the async registrations:
-pub async fn setup(engine: Arc<dyn Engine>, mut udev: UdevHandler) -> StratisResult<()> { - { - let pools = engine.pools().await; - for (_, pool_uuid, _) in pools.iter() { - if let Err(e) = udev.register_pool(*pool_uuid).await { - warn!("Failed to register existing pools: {e}"); - } - } - } +pub async fn setup(engine: Arc<dyn Engine>, mut udev: UdevHandler) -> StratisResult<()> { + let pool_uuids = { + let pools = engine.pools().await; + pools + .iter() + .map(|(_, pool_uuid, _)| *pool_uuid) + .collect::<Vec<_>>() + }; + + for pool_uuid in pool_uuids { + if let Err(e) = udev.register_pool(pool_uuid).await { + warn!("Failed to register existing pools: {e}"); + } + }Also note that if
process_udev_events()returns an error, the loop breaks andsetupultimately returnsOk(())(only logging the error). If you want callers to treat a failed udev loop as fatal, you may want to propagate that error instead of just logging and exiting the loop.src/dbus/manager/mod.rs (1)
43-62: Bidirectional map invariant can be violated.The
add_*methods don't handle cases where the UUID or path already exists with a different counterpart. This can break the bidirectional invariant and lead to stale mappings. Additionally,path.clone()is called twice per insertion—an owned clone could be shared.src/dbus/manager/manager_3_0/methods.rs (2)
167-177: Release the manager lock before awaiting destroy/unregister.The write guard from
manager.write().awaitis held across async awaits (engine.destroy_poolandunregister_pool). This can deadlock becauseunregister_poolacquiresmanager.write().awaitagain. Scope the lock to a short block or explicitly drop it after obtaining the UUID.Apply this diff:
- let manager_lock = manager.write().await; - let uuid = match manager_lock.pool_get_uuid(&pool) { - Some(u) => u, - None => { - return ( - default_return, - DbusErrorEnum::ERROR as u16, - "Object path {pool} not associated with pool".to_string(), - ); - } - }; + let uuid = { + let manager_guard = manager.write().await; + match manager_guard.pool_get_uuid(&pool) { + Some(u) => u, + None => { + return ( + default_return, + DbusErrorEnum::ERROR as u16, + format!("Object path {} not associated with pool", pool), + ); + } + } + };
174-174: Fix string interpolation bug in error message.Line 174 contains a literal
"{pool}"string that should interpolate the object path value. Useformat!to build the error message.This is addressed in the diff above.
🧹 Nitpick comments (1)
src/stratis/errors.rs (1)
150-156: LGTM with minor formatting note.The Display implementations correctly handle both the
DbusandDbusTypevariants with clear, descriptive messages. The distinction between "Dbus error" and "Dbus type error" helps differentiate general D-Bus errors from type conversion errors.Minor nitpick: The trailing commas after the format strings on lines 151 and 155 are unnecessary:
- write!(f, "Dbus error: {err}",) + write!(f, "Dbus error: {err}")- write!(f, "Dbus type error: {err}",) + write!(f, "Dbus type error: {err}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
Cargo.toml(2 hunks)src/dbus/consts.rs(1 hunks)src/dbus/filesystem/filesystem_3_0/mod.rs(1 hunks)src/dbus/filesystem/filesystem_3_0/props.rs(1 hunks)src/dbus/filesystem/filesystem_3_9/mod.rs(1 hunks)src/dbus/filesystem/mod.rs(1 hunks)src/dbus/filesystem/shared.rs(1 hunks)src/dbus/macros.rs(1 hunks)src/dbus/manager/manager_3_0/methods.rs(1 hunks)src/dbus/manager/manager_3_0/mod.rs(1 hunks)src/dbus/manager/manager_3_0/props.rs(1 hunks)src/dbus/manager/manager_3_1/mod.rs(1 hunks)src/dbus/manager/manager_3_8/methods.rs(1 hunks)src/dbus/manager/manager_3_8/mod.rs(1 hunks)src/dbus/manager/manager_3_9/mod.rs(1 hunks)src/dbus/manager/mod.rs(1 hunks)src/dbus/mod.rs(1 hunks)src/dbus/pool/mod.rs(1 hunks)src/dbus/pool/pool_3_0/mod.rs(1 hunks)src/dbus/pool/pool_3_0/props.rs(1 hunks)src/dbus/pool/pool_3_6/methods.rs(1 hunks)src/dbus/pool/pool_3_6/mod.rs(1 hunks)src/dbus/pool/pool_3_9/mod.rs(1 hunks)src/dbus/pool/shared.rs(1 hunks)src/dbus/types.rs(1 hunks)src/dbus/udev.rs(1 hunks)src/dbus/util.rs(1 hunks)src/engine/engine.rs(1 hunks)src/engine/mod.rs(1 hunks)src/engine/sim_engine/pool.rs(5 hunks)src/engine/strat_engine/pool/dispatch.rs(1 hunks)src/engine/strat_engine/pool/v1.rs(2 hunks)src/engine/strat_engine/pool/v2.rs(2 hunks)src/engine/types/actions.rs(2 hunks)src/engine/types/keys.rs(4 hunks)src/engine/types/mod.rs(4 hunks)src/stratis/dm.rs(4 hunks)src/stratis/errors.rs(3 hunks)src/stratis/ipc_support/dbus_support.rs(1 hunks)src/stratis/ipc_support/jsonrpc_support.rs(2 hunks)src/stratis/run.rs(2 hunks)src/stratis/timer.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- src/stratis/ipc_support/jsonrpc_support.rs
- src/engine/mod.rs
- src/dbus/pool/pool_3_0/props.rs
- src/dbus/manager/manager_3_0/props.rs
- src/dbus/udev.rs
- src/dbus/consts.rs
- src/dbus/filesystem/filesystem_3_0/mod.rs
- src/dbus/macros.rs
- src/dbus/util.rs
- src/engine/types/actions.rs
- src/dbus/pool/shared.rs
- src/dbus/pool/pool_3_6/mod.rs
- src/engine/strat_engine/pool/v2.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
Repo: stratis-storage/stratisd PR: 3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/dbus/manager/manager_3_8/mod.rssrc/stratis/ipc_support/dbus_support.rssrc/dbus/types.rssrc/dbus/pool/pool_3_0/mod.rssrc/stratis/dm.rssrc/dbus/manager/manager_3_0/methods.rssrc/stratis/timer.rssrc/engine/engine.rssrc/dbus/pool/mod.rssrc/dbus/manager/manager_3_8/methods.rssrc/dbus/pool/pool_3_9/mod.rssrc/dbus/manager/manager_3_1/mod.rssrc/engine/strat_engine/pool/v1.rssrc/dbus/manager/manager_3_9/mod.rssrc/dbus/manager/mod.rssrc/dbus/manager/manager_3_0/mod.rssrc/dbus/pool/pool_3_6/methods.rs
📚 Learning: 2025-09-05T14:32:42.528Z
Learnt from: jbaublitz
Repo: stratis-storage/stratisd PR: 3898
File: src/engine/sim_engine/pool.rs:934-948
Timestamp: 2025-09-05T14:32:42.528Z
Learning: In src/engine/sim_engine/pool.rs, the start_encrypt_pool method intentionally creates a SizedKeyMemory with 1 byte allocated but 0 length reported - this is to avoid NULL pointer issues while simulating an empty key slice in the sim engine.
Applied to files:
src/dbus/manager/manager_3_8/mod.rssrc/dbus/manager/manager_3_0/methods.rssrc/engine/sim_engine/pool.rssrc/dbus/manager/manager_3_8/methods.rs
🧬 Code graph analysis (28)
src/dbus/manager/manager_3_8/mod.rs (1)
src/dbus/manager/manager_3_8/methods.rs (1)
create_pool_method(32-130)
src/dbus/filesystem/filesystem_3_0/props.rs (2)
src/dbus/pool/pool_3_9/mod.rs (1)
Name(104-106)src/dbus/filesystem/filesystem_3_9/mod.rs (1)
Name(54-56)
src/dbus/filesystem/shared.rs (2)
src/dbus/pool/pool_3_9/mod.rs (2)
Name(104-106)Uuid(98-100)src/dbus/filesystem/filesystem_3_9/mod.rs (2)
Name(54-56)Uuid(48-50)
src/stratis/ipc_support/dbus_support.rs (4)
src/stratis/ipc_support/jsonrpc_support.rs (1)
setup(39-56)src/engine/engine.rs (1)
pools(507-507)src/engine/strat_engine/engine.rs (2)
pools(201-203)pools(739-741)src/engine/sim_engine/engine.rs (1)
pools(300-302)
src/dbus/types.rs (2)
src/dbus/util.rs (1)
result_option_to_tuple(42-55)src/engine/sim_engine/pool.rs (1)
new(58-77)
src/dbus/filesystem/mod.rs (3)
src/dbus/pool/pool_3_9/mod.rs (2)
connection(73-73)register(49-67)src/dbus/manager/manager_3_0/mod.rs (1)
register(53-70)src/dbus/filesystem/filesystem_3_9/mod.rs (1)
register(30-41)
src/dbus/mod.rs (7)
src/dbus/pool/pool_3_9/mod.rs (2)
connection(73-73)new(33-47)src/dbus/util.rs (4)
connection(76-78)connection(89-91)send_fs_background_signals(107-113)send_pool_background_signals(68-104)src/dbus/manager/mod.rs (1)
register_manager(110-124)src/dbus/manager/manager_3_0/mod.rs (1)
new(39-51)src/dbus/manager/manager_3_9/mod.rs (1)
new(29-41)src/dbus/udev.rs (1)
new(25-39)src/engine/structures/lock.rs (1)
new_shared(84-86)
src/dbus/pool/pool_3_0/mod.rs (1)
src/dbus/pool/pool_3_0/props.rs (4)
allocated_prop(29-31)name_prop(12-15)size_prop(17-19)used_prop(21-27)
src/stratis/run.rs (5)
src/dbus/mod.rs (1)
create_dbus_handler(32-53)src/stratis/ipc_support/dbus_support.rs (1)
setup(23-51)src/stratis/ipc_support/jsonrpc_support.rs (1)
setup(39-56)src/stratis/dm.rs (1)
dm_event_thread(36-110)src/stratis/timer.rs (1)
run_timers(74-88)
src/stratis/dm.rs (1)
src/dbus/util.rs (4)
send_fs_background_signals(107-113)send_pool_background_signals(68-104)connection(76-78)connection(89-91)
src/dbus/manager/manager_3_0/methods.rs (3)
src/dbus/pool/mod.rs (2)
register_pool(29-54)unregister_pool(56-70)src/dbus/util.rs (4)
engine_to_dbus_err_tuple(59-65)tuple_to_option(19-25)connection(76-78)connection(89-91)src/engine/types/keys.rs (13)
new(46-48)new(126-144)new(278-282)new(728-730)key_desc(84-89)clevis_info(91-96)clevis_info(628-641)from(251-258)from(645-661)from(669-677)from(813-820)new_legacy(105-124)uuid(732-734)
src/stratis/errors.rs (4)
src/dbus_api/filesystem/filesystem_3_0/api.rs (7)
f(33-33)f(40-40)f(47-47)f(54-54)f(61-61)f(68-68)f(75-75)src/dbus_api/blockdev/blockdev_3_0/api.rs (5)
f(34-34)f(41-41)f(48-48)f(57-57)f(64-64)src/dbus_api/pool/pool_3_1/api.rs (3)
f(17-17)f(28-28)f(39-39)src/dbus/types.rs (1)
from(42-105)
src/engine/strat_engine/pool/dispatch.rs (6)
src/engine/engine.rs (1)
create_filesystems(162-167)src/engine/sim_engine/pool.rs (1)
create_filesystems(221-263)src/engine/strat_engine/pool/v1.rs (1)
create_filesystems(846-895)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)src/dbus_api/pool/pool_3_0/methods.rs (1)
create_filesystems(29-129)src/engine/shared.rs (1)
specs(248-256)
src/engine/sim_engine/pool.rs (7)
src/engine/engine.rs (3)
create_filesystems(162-167)size(93-93)size(122-122)src/engine/strat_engine/pool/dispatch.rs (1)
create_filesystems(111-121)src/engine/strat_engine/pool/v1.rs (1)
create_filesystems(846-895)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)src/dbus_api/pool/pool_3_6/methods.rs (1)
create_filesystems(21-130)src/engine/types/mod.rs (6)
new(178-180)new(277-279)new(310-322)new(375-377)as_ref(188-190)as_ref(412-417)src/engine/types/actions.rs (3)
new(235-237)new(311-313)new(586-589)
src/stratis/timer.rs (2)
src/dbus/util.rs (4)
send_fs_background_signals(107-113)send_pool_background_signals(68-104)connection(76-78)connection(89-91)src/dbus/pool/pool_3_9/mod.rs (1)
connection(73-73)
src/engine/engine.rs (4)
src/engine/sim_engine/pool.rs (1)
create_filesystems(221-263)src/engine/strat_engine/pool/dispatch.rs (1)
create_filesystems(111-121)src/engine/strat_engine/pool/v1.rs (1)
create_filesystems(846-895)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)
src/engine/types/mod.rs (2)
src/dbus/pool/pool_3_9/mod.rs (2)
Uuid(98-100)Name(104-106)src/dbus/types.rs (1)
from(42-105)
src/dbus/pool/mod.rs (2)
src/dbus/udev.rs (1)
register_pool(62-72)src/dbus/pool/pool_3_9/mod.rs (3)
connection(73-73)register(49-67)unregister(69-75)
src/dbus/manager/manager_3_8/methods.rs (3)
src/dbus/pool/mod.rs (1)
register_pool(29-54)src/dbus/util.rs (4)
engine_to_dbus_err_tuple(59-65)tuple_to_option(19-25)connection(76-78)connection(89-91)src/engine/types/keys.rs (12)
new(46-48)new(126-144)new(278-282)new(728-730)from(251-258)from(645-661)from(669-677)from(813-820)try_from(499-521)try_from(705-713)try_from(719-721)uuid(732-734)
src/dbus/pool/pool_3_9/mod.rs (3)
src/dbus/pool/pool_3_0/props.rs (4)
allocated_prop(29-31)name_prop(12-15)size_prop(17-19)used_prop(21-27)src/dbus/pool/pool_3_6/methods.rs (1)
create_filesystems_method(25-124)src/dbus/pool/shared.rs (2)
pool_prop(21-29)try_pool_prop(31-39)
src/engine/types/keys.rs (2)
src/engine/strat_engine/liminal/device_info.rs (11)
into(74-101)into(194-221)into(268-273)into(773-775)into(804-806)from(61-68)from(125-133)from(182-188)from(257-262)from(472-480)from(484-492)src/dbus/types.rs (1)
from(42-105)
src/dbus/manager/manager_3_1/mod.rs (1)
src/dbus/manager/manager_3_0/props.rs (1)
version_prop(12-14)
src/engine/strat_engine/pool/v1.rs (5)
src/engine/engine.rs (3)
create_filesystems(162-167)size(93-93)size(122-122)src/engine/sim_engine/pool.rs (2)
create_filesystems(221-263)new(58-77)src/engine/strat_engine/pool/dispatch.rs (1)
create_filesystems(111-121)src/engine/strat_engine/pool/v2.rs (1)
create_filesystems(751-800)src/dbus_api/pool/pool_3_0/methods.rs (1)
create_filesystems(29-129)
src/dbus/manager/manager_3_9/mod.rs (3)
src/dbus/manager/manager_3_0/props.rs (1)
version_prop(12-14)src/dbus/manager/manager_3_8/methods.rs (4)
create_pool_method(32-130)devs(47-47)key_desc(48-51)clevis_info(52-52)src/dbus/manager/manager_3_0/mod.rs (3)
new(39-51)register(53-70)CreatePool(109-128)
src/dbus/manager/mod.rs (4)
src/dbus/pool/pool_3_9/mod.rs (2)
connection(73-73)register(49-67)src/dbus/manager/manager_3_1/mod.rs (1)
register(51-68)src/dbus/manager/manager_3_0/mod.rs (1)
register(53-70)src/dbus/manager/manager_3_9/mod.rs (1)
register(43-60)
src/dbus/manager/manager_3_0/mod.rs (2)
src/dbus/manager/manager_3_0/methods.rs (7)
create_pool_method(99-157)destroy_pool_method(159-204)list_keys_method(34-44)set_key_method(46-74)unlock_pool_method(206-227)unset_key_method(76-96)devs(111-111)src/dbus/manager/manager_3_0/props.rs (2)
locked_pools_prop(16-18)version_prop(12-14)
src/dbus/filesystem/filesystem_3_9/mod.rs (2)
src/dbus/filesystem/filesystem_3_0/props.rs (1)
name_prop(7-9)src/dbus/filesystem/shared.rs (1)
filesystem_prop(11-27)
src/dbus/pool/pool_3_6/methods.rs (3)
src/dbus/filesystem/mod.rs (1)
register_filesystem(25-43)src/engine/engine.rs (2)
filesystems(317-317)create_filesystems(162-167)src/engine/types/actions.rs (13)
changed(35-35)changed(54-59)changed(176-181)changed(200-205)changed(262-273)changed(327-333)changed(396-401)changed(518-523)changed(606-612)changed(675-680)changed(724-729)changed(759-764)changed(829-834)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (32)
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: checks (make -f Makefile test, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile build, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-no-ipc, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile docs-ci, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile build-min, 1.91.0, cargo)
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-utils, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile build, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-no-ipc, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile docs-ci, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile test, 1.91.0, cargo)
- GitHub Check: tests-with-testing-repo (master)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min-no-systemd, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.91.0, clippy)
- GitHub Check: python-based-tests
- GitHub Check: stratis-min-cli-checks
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.91.0, cargo)
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
🔇 Additional comments (45)
src/stratis/errors.rs (1)
231-242: Code changes are correct and verified.The verification confirms that:
- Pattern matches on
StratisError::DbusandStratisError::DbusTypeonly exist in the Display implementation (lines 150 and 154), and both correctly useref errsyntax for non-Arc types- No pattern matches on these variants exist outside
errors.rs, so no updates are needed elsewhere- The
Fromimplementations are correctly implemented to enable automatic error propagation via the?operatorDbusTypeis appropriately used forzbus::zvariant::Errorconversionssrc/dbus/types.rs (3)
14-14: LGTM: Type alias clearly documents D-Bus filesystem spec structure.The
FilesystemSpecalias effectively captures the D-Bus tuple structure (name, (exists, size), (exists, size_limit)) used across manager/pool interfaces.
16-21: LGTM: Simple error enum for D-Bus error codes.The enum provides explicit OK/ERROR values for D-Bus method returns.
56-77: warn-and-continue pattern is appropriate for optional D-Bus fields.The code at lines 56-77 serializes optional encryption metadata (key_description and clevis_info) to D-Bus variant dictionaries. The warn-and-continue approach is safe because:
D-Bus variant dictionaries (a{sv}) inherently support optional/missing keys — clients iterate over present keys and handle absence gracefully. Missing keys are not an error condition.
result_option_to_tuple()already wraps errors — it returns(bool, Value)tuples, converting errors to strings; ifdict.add()fails, the issue is serialization, not missing optional data.No mandatory field requirements found — no consumers use
.expect(),.unwrap(), or validation requiring these fields; the pattern is consistent across src/dbus_api/api/prop_conv.rs.Incomplete serialization is safe — if a key fails to add, it's simply absent from the dictionary; clients handle this by checking key presence rather than assuming it.
src/dbus/manager/manager_3_8/mod.rs (1)
1-7: LGTM: Clean module organization.Straightforward submodule declaration and re-export for the manager interface.
src/dbus/pool/pool_3_0/mod.rs (1)
1-7: LGTM: Clean props re-export.Standard pattern for exposing pool property accessors.
src/dbus/filesystem/filesystem_3_0/props.rs (1)
7-9: LGTM: Identity accessor for Name property.The function signature matches the callback pattern used by
filesystem_prophelpers (see src/dbus/filesystem/shared.rs), even though only thenameparameter is used here.Cargo.toml (2)
292-292: LGTM: Feature update aligns with dependency changes.The
dbus_enabledfeature correctly references the new zbus dependency.
233-236: I need to verify the zbus 5.12.0 release details and tokio feature support. Let me search for more specific information.zbus 5.12.0 is stable and verified compatible with tokio runtime.
zbus 5.12.0 was published recently with no widely reported stability regressions. zbus is runtime-agnostic and documented to work out of the box with different Rust async runtimes, including tokio. The tokio feature is explicitly supported and documented in zbus 5.x with no known incompatibilities identified.
src/dbus/filesystem/shared.rs (1)
11-27: LGTM: Clean property accessor with good error messages.The generic callback pattern allows flexible property extraction while maintaining clear error handling.
src/engine/sim_engine/pool.rs (3)
221-226: LGTM: Signature updated to return Name type.The function signature correctly reflects the migration from
&strtoNamefor filesystem names, aligning with the broader type safety improvements across the engine.
258-258: LGTM: Correct Name wrapping.Line 258 properly wraps the owned name string in
Name::new()to match the updated return type.
1137-1137: LGTM: Test assertions correctly access Name type.Tests properly access the Name value via tuple field
.0and call.as_ref()to obtain the&strfor comparison.Also applies to: 1276-1276, 1330-1330
src/engine/types/mod.rs (4)
20-21: LGTM!The zbus imports are correctly feature-gated and provide the necessary types for D-Bus serialization.
58-106: LGTM!The uuid! macro correctly adds zbus Type derives and Value conversion under the dbus_enabled feature. The signature annotation "s" is appropriate for string representation of UUIDs.
168-175: LGTM!The Name type correctly derives zbus Type and Value traits when dbus_enabled is active, with the appropriate signature "s" for string serialization.
109-109: Type change verified — no compatibility issues found.The change from
(String, Value)to(String, serde_json::Value)is a safe disambiguation that makes the JSON value type explicit. All 90+ usages across the codebase—including destructuring patterns, enum variants, function parameters, and serialization operations intypes/keys.rs,sim_engine/pool.rs, andstrat_engine/modules—remain compatible. The actual runtime behavior and serialization characteristics are unchanged.src/dbus/filesystem/mod.rs (1)
25-43: LGTM!The filesystem registration flow is correct: generates unique path using atomic counter, registers with D-Bus, updates manager state, and returns the path. The AcqRel ordering ensures proper synchronization.
src/engine/engine.rs (1)
162-167: LGTM!The signature change correctly removes the unnecessary lifetime parameter and uses the new Name type in the return value. This aligns with the broader refactoring to introduce strongly-typed names throughout the codebase.
src/stratis/run.rs (1)
100-130: LGTM!The IPC initialization correctly migrates from channel-based communication to the new zbus connection and manager approach. The feature gating is consistent, and parameters are properly threaded through to dm_event_thread and run_timers.
src/engine/strat_engine/pool/dispatch.rs (1)
111-120: LGTM!The signature update correctly propagates the create_filesystems changes to the dispatch layer, maintaining consistent delegation to V1/V2 pool implementations.
src/stratis/dm.rs (2)
36-43: LGTM!The function signature correctly adds connection and manager parameters under the dbus_enabled feature, aligning with the new D-Bus architecture that uses direct signal emission instead of channel-based communication.
66-74: LGTM!The replacement of channel-based communication with direct background signal emission is correct. Error handling is appropriate with warning logs.
src/dbus/pool/pool_3_6/methods.rs (1)
69-83: Verify spawn_blocking usage for pool operations.The code uses
spawn_blockingto execute the synchronous pool operation. Confirm that pool.create_filesystems performs blocking I/O operations that justify moving to a blocking thread, as this adds overhead.src/dbus/manager/manager_3_8/methods.rs (2)
47-92: LGTM!Input normalization and integrity spec construction are correct. The code properly handles optional values, parses JSON, and validates the integrity tag specification with appropriate error mapping.
93-129: LGTM!The pool creation flow is correct: creates pool via engine, registers on success, and properly maps all outcomes to D-Bus return values. Error handling is consistent with the established patterns.
src/engine/strat_engine/pool/v1.rs (1)
845-895: create_filesystems v1 correctly aligned with Name-based APIThe updated signature and the
Name::new(name.to_string())wrapping in the result match the new Engine trait and the v2 implementation; size/overprov and duplicate-name checks remain unchanged and look correct.src/dbus/filesystem/filesystem_3_9/mod.rs (1)
1-57: New FilesystemR9 zbus interface looks consistent and safeThe struct layout,
registerwiring, and theUuid/Nameproperties all match the existing shared helpers (filesystem_prop,name_prop) and avoid holding engine locks across additional awaits; no issues spotted.src/dbus/mod.rs (1)
1-53: create_dbus_handler cleanly encapsulates D‑Bus setupThe helper centralizes system bus connection creation, manager registration, and
UdevHandlerconstruction in a straightforward way; Arc/Lockable usage and ordering (build connection → register manager → build handler) look correct.src/dbus/pool/pool_3_9/mod.rs (1)
24-125: PoolR9 D‑Bus interface correctly wired to shared helpersThe PoolR9 struct, its
register/unregistermethods,CreateFilesystemsdelegation, and the property implementations all align with existing helpers (create_filesystems_method,pool_prop/try_pool_prop, *_prop functions) and avoid holding engine locks across additional awaits; the design looks sound.src/stratis/timer.rs (3)
7-18: LGTM!The feature-gated imports and module organization are clean and consistent with the broader zbus migration.
22-69: LGTM!The timer implementation correctly threads the new D-Bus connection and manager through the async processing flow. The feature gating ensures compatibility with both D-Bus-enabled and minimal builds, and the use of cloned Arc references in the async task is appropriate.
74-88: LGTM!The updated
run_timerssignature correctly propagates the connection and manager to the spawned task. The feature-gated parameters align with the broader migration pattern.src/dbus/manager/manager_3_1/mod.rs (1)
1-141: LGTM!The ManagerR1 implementation follows the established pattern from ManagerR0 and ManagerR9. The interface correctly delegates to shared methods in manager_3_0, and the async registration flow is properly structured.
src/dbus/manager/manager_3_9/mod.rs (1)
1-99: LGTM!The ManagerR9 implementation is consistent with the other manager versions. It correctly delegates to the manager_3_8::create_pool_method for the more advanced pool creation API with additional parameters (journal_size, tag_spec, allocate_superblock).
src/dbus/manager/mod.rs (2)
88-108: LGTM!The removal methods correctly maintain the bidirectional invariant by removing entries from both maps.
110-124: LGTM!The
register_managerfunction correctly registers all manager versions (R0, R1, R9) and binds the ObjectManager to the base path.src/engine/types/keys.rs (2)
470-494: LGTM!The conversion to
serde_json::Valueis correctly implemented. The mapping fromUnlockMechanismto JSON values (strings for KeyDesc, arrays for ClevisInfo) follows the expected pattern.
681-758: LGTM!The feature-gated declarations for
KeyDescriptionandUnlockMethodcorrectly add zbusTypeandValuederives when D-Bus is enabled, maintaining compatibility with non-D-Bus builds.src/dbus/manager/manager_3_0/methods.rs (5)
34-44: LGTM!The
list_keys_methodcorrectly delegates to the engine's key handler and translates errors to D-Bus format.
46-74: LGTM!The
set_key_methodproperly handles the file descriptor and translates theMappingCreateActionvariants to appropriate D-Bus return values.
76-96: LGTM!The
unset_key_methodcorrectly handles key removal and error translation.
98-157: LGTM!The
create_pool_methodcorrectly parses encryption info, invokes the engine's create_pool, and registers the pool on success. The error handling and default return patterns are appropriate.
206-227: LGTM!The
unlock_pool_methodcorrectly delegates to the engine and translates the unlock action results.src/dbus/manager/manager_3_0/mod.rs (1)
1-143: LGTM!The ManagerR0 implementation follows the established pattern and correctly delegates to shared method implementations. The D-Bus interface is properly structured with appropriate property signals and async method wiring.
jbaublitz
force-pushed
the
issue-stratisd-2574
branch
from
6f88a9c to
f52451b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/dbus/manager/mod.rs (1)
45-64: Bidirectional map invariant can be violated.The
add_*methods don't handle cases where the UUID or path already exists with a different counterpart, which can break the bidirectional invariant.Scenario:
add_pool(path1, uuid1) // path1→uuid1, uuid1→path1 ✓ add_pool(path2, uuid1) // path2→uuid1, uuid1→path2 // Now: path1→uuid1 (stale), path2→uuid1, uuid1→path2 // Invariant broken: path1 maps to uuid1, but uuid1 doesn't map back to path1Additionally,
path.clone()is called twice per insertion—an owned clone could be shared.Apply this diff to
add_poolto remove stale entries and eliminate the double clone:pub fn add_pool(&mut self, path: &ObjectPath<'_>, uuid: PoolUuid) { - self.pool_path_to_uuid - .insert(OwnedObjectPath::from(path.clone()), uuid); - self.pool_uuid_to_path - .insert(uuid, OwnedObjectPath::from(path.clone())); + let owned_path = OwnedObjectPath::from(path.clone()); + + // Remove stale reverse mapping if UUID was previously mapped to a different path + if let Some(old_path) = self.pool_uuid_to_path.get(&uuid) { + if old_path != &owned_path { + self.pool_path_to_uuid.remove(old_path); + } + } + + // Remove stale forward mapping if path was previously mapped to a different UUID + if let Some(old_uuid) = self.pool_path_to_uuid.get(&owned_path) { + if old_uuid != &uuid { + self.pool_uuid_to_path.remove(old_uuid); + } + } + + self.pool_path_to_uuid.insert(owned_path.clone(), uuid); + self.pool_uuid_to_path.insert(uuid, owned_path); }Apply similar fixes to
add_filesystemandadd_blockdev.src/dbus/util.rs (1)
114-121: Filesystem background signals helper remains a no-op stub
send_fs_background_signalsstill ignores itsmanager,connection, anddiffsarguments and always returnsOk(()). If callers expect filesystem property-change notifications (similar to pools), this will silently drop them.If this is intentional scaffolding, consider either:
- Implementing the analogous
FilesystemR9signal emission, or- Adding a TODO/tracking-issue reference in the function doc/comment explaining that FS diffs are currently not signaled.
src/dbus/manager/manager_3_0/methods.rs (2)
167-177: Fix string interpolation bug in error message.Line 174 uses a literal
{pool}in the error message instead of interpolating the actual pool path value.Apply this diff:
- "Object path {pool} not associated with pool".to_string(), + format!("Object path {} not associated with pool", pool),
167-206: Release the manager lock before awaiting destroy/unregister.The write lock acquired at line 167 is held across the
engine.destroy_poolawait (line 179) and theunregister_poolawait (line 181). Sinceunregister_poolinternally callsmanager.write().await(see src/dbus/pool/mod.rs:59), this causes a deadlock.Apply this diff to scope the lock acquisition:
- let manager_lock = manager.write().await; - let uuid = match manager_lock.pool_get_uuid(&pool) { - Some(u) => u, - None => { - return ( - default_return, - DbusErrorEnum::ERROR as u16, - "Object path {pool} not associated with pool".to_string(), - ); - } - }; + let uuid = { + let manager_lock = manager.write().await; + match manager_lock.pool_get_uuid(&pool) { + Some(u) => u, + None => { + return ( + default_return, + DbusErrorEnum::ERROR as u16, + format!("Object path {} not associated with pool", pool), + ); + } + } + };
🧹 Nitpick comments (8)
src/dbus/util.rs (2)
43-63: Avoid constructingdefaulton the fast path inresult_option_to_tupleCurrent implementation always builds
default: Teven whenresultisOk(Some(_)), where it’s unused. This is minor but avoidable overhead (e.g., extraString/tuple allocations) on the success path.You could pattern-match explicitly to only construct
defaultwhen needed:-pub fn result_option_to_tuple<'a, T, E>( - result: Result<Option<T>, E>, - default: T, -) -> (bool, Value<'a>) +pub fn result_option_to_tuple<'a, T, E>( + result: Result<Option<T>, E>, + default: T, +) -> (bool, Value<'a>) where E: Display, Value<'a>: From<T> + From<(bool, T)>, { - let (success, value) = match result { - Ok(value) => (true, Value::from(option_to_tuple(value, default))), - Err(e) => (false, Value::from(e.to_string())), - }; + let (success, value) = match result { + Ok(Some(v)) => (true, Value::from((true, v))), + Ok(None) => (true, Value::from((false, default))), + Err(e) => (false, Value::from(e.to_string())), + }; (success, value) }
75-109: Narrow theManagerread-lock scope and avoid doublePoolR9lookup per pool
send_pool_background_signalscurrently:
- Holds a
manager.read().awaitguard across the whole loop and subsequentawaits, and- Re-derives the
PoolR9interface twice when bothallocated_sizeandusedchanged.This works, but is suboptimal for contention and allocs. You can:
- Limit the lock scope to just computing the
ObjectPath, and- Fetch the
PoolR9interface once per(uuid, diff)when any relevant field changed.Sketch:
- let dbus = manager.read().await; - for (uuid, diff) in diffs { - if diff.thin_pool.allocated_size.changed().is_some() { - let iface_ref = connection - .object_server() - .interface::<_, PoolR9>(dbus.pool_get_path(&uuid).ok_or_else(|| { - StratisError::Msg(format!("No pool associated with UUID {uuid}")) - })?) - .await?; - ... - } - if diff.thin_pool.used.changed().is_some() { - let iface_ref = connection - .object_server() - .interface::<_, PoolR9>(dbus.pool_get_path(&uuid).ok_or_else(|| { - StratisError::Msg(format!("No pool associated with UUID {uuid}")) - })?) - .await?; - ... - } - } + for (uuid, diff) in diffs { + let allocated_changed = diff.thin_pool.allocated_size.changed().is_some(); + let used_changed = diff.thin_pool.used.changed().is_some(); + if !(allocated_changed || used_changed) { + continue; + } + + let path = { + let dbus = manager.read().await; + dbus.pool_get_path(&uuid) + .ok_or_else(|| StratisError::Msg(format!("No pool associated with UUID {uuid}")))? + }; + + let iface_ref = connection.object_server().interface::<_, PoolR9>(path).await?; + let mut iface = iface_ref.get_mut().await; + if allocated_changed { + iface.allocated_size_changed(iface_ref.signal_emitter()).await?; + } + if used_changed { + iface.total_physical_used_changed(iface_ref.signal_emitter()).await?; + } + }This keeps the async DBus calls outside the lock and avoids redundant interface lookups.
src/dbus/manager/manager_3_2/methods.rs (2)
28-129: Tighten pool guard lifetime and consider partial-registration behavior instart_pool_methodThe overall flow and error mapping look sound, but two details are worth considering:
Guard held across async calls
guardfromengine.get_pool(...)is kept alive while youawaitmultipleregister_filesystem/register_poolcalls and signal emissions. Ifguardinternally holds engine state locks, this can unnecessarily constrain concurrency.You can snapshot what you need before awaiting, then drop the guard:
Ok(StartAction::Started(_)) => {let guard = match engine.get_pool(PoolIdentifier::Uuid(pool_uuid)).await {
Ok(StartAction::Started(_)) => {let guard = match engine.get_pool(PoolIdentifier::Uuid(pool_uuid)).await { Some(g) => g, None => { ... } };
let mut fs_paths = Vec::default();for fs_uuid in guard.filesystems().into_iter().map(|(_, fs_uuid, _)| fs_uuid).collect::<Vec<_>>(){
let encrypted = guard.is_encrypted();let fs_uuids = guard.filesystems().into_iter().map(|(_, fs_uuid, _)| fs_uuid).collect::<Vec<_>>();drop(guard);let mut fs_paths = Vec::default();for fs_uuid in fs_uuids { let fs_path = match register_filesystem( engine, connection, manager, counter, pool_uuid, fs_uuid, )
Partial registration rollback
If anyregister_filesystemorregister_poolcall fails, you return(default_return, rc, rs)without attempting to unregister any FSes or pool objects created earlier in the loop. This may leave partially-registered DBus objects visible.If preserving the pre-migration behavior isn’t required here, consider a best-effort rollback (unregister anything created so far) or at least documenting that partial registration is possible in error paths.
131-200: Clarify semantics forStopPoolon partial teardown
stop_pool_methodlooks consistent overall (UUID resolution, engine action, unregister, and signal emission). Two contract points might be worth double‑checking:
- For
StopAction::Stopped(_) | StopAction::Partial(_)you eagerly callunregister_pooland emitStoppedPools(and possiblyLockedPools) signals, so from DBus clients’ perspective the pool is gone in both cases.- In the final match,
StopAction::Partial(_)maps toDbusErrorEnum::ERRORwith a descriptive message, whileStopAction::Stopped(_)maps toOK.This combination (object unregistered but error code non‑zero) is subtle. If it matches the previous dbus‑tree API semantics, great; otherwise, consider whether callers would prefer either:
- Keep the current behavior but document that ERROR +
(false, PoolUuid::default())still means “pool is gone but teardown was incomplete”, or- Use a distinct
DbusErrorEnum/code for partial teardown if you later extend the enum.src/dbus/types.rs (1)
113-183: Fix log messages instopped_pools_to_valueto reference stopped pools, not locked pools
stopped_pools_to_valuecorrectly mirrors the structure used forLockedPoolsInfo, but all thewarn!messages still say:"Failed to convert locked pool information to D-Bus format: {e}"
That’s misleading when debugging issues related to stopped or partially constructed pools.
Consider updating the messages here to reference “stopped pool information” (or similar) to distinguish them from the locked‑pools conversions.
src/engine/types/keys.rs (2)
470-493: JSON conversion forEncryptionInfomatches existing usage; only minor cloning overheadThe new
impl Into<serde_json::Value> for &EncryptionInfocleanly produces the expected JSON object:
- Keys are token slot numbers as strings.
KeyDescentries become plain strings (application‑level key descriptions).ClevisInfoentries become[pin, config]arrays.This aligns with the
device_infocode that merges&EncryptionInfoJSON into larger objects. The extraconfig.to_owned()clone per entry is acceptable given the small expected size, but if configs ever become large you could switch to building the array viaserde_json::Value::Arrayand movingconfiginstead.
680-758: Feature-gatedKeyDescription/UnlockMethodDBus typing is well-scopedUnder
dbus_enabled, derivingTypeandValuewith#[zvariant(signature = "s")]forKeyDescriptionandUnlockMethodcleanly exposes them as string‑typed DBus values while keeping the non‑DBus build free of zbus dependencies.One minor readability nit: the comment above
impl Hash for EncryptionInfostill refers generically to “Value”, which might now be ambiguous withzbus::zvariant::Valuealso in scope underdbus_enabled. If you touch this area again, consider clarifying that it refers toserde_json::Value.src/dbus/manager/manager_3_0/methods.rs (1)
183-183: Consider returning an error instead of panicking on UUID mismatch.The
assert_eq!will panic if the UUIDs don't match, which crashes the D-Bus service. While this check catches internal inconsistencies, consider logging an error and returning a D-Bus error instead to avoid service disruption.Example approach:
if uuid != u { error!("UUID mismatch: destroy_pool returned {}, unregister_pool returned {}", uuid, u); return ( default_return, DbusErrorEnum::ERROR as u16, "Internal error: UUID mismatch".to_string(), ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/dbus/filesystem/mod.rs(1 hunks)src/dbus/manager/manager_3_0/methods.rs(1 hunks)src/dbus/manager/manager_3_0/mod.rs(1 hunks)src/dbus/manager/manager_3_0/props.rs(1 hunks)src/dbus/manager/manager_3_1/mod.rs(1 hunks)src/dbus/manager/manager_3_2/methods.rs(1 hunks)src/dbus/manager/manager_3_2/mod.rs(1 hunks)src/dbus/manager/manager_3_2/props.rs(1 hunks)src/dbus/manager/mod.rs(1 hunks)src/dbus/mod.rs(1 hunks)src/dbus/pool/mod.rs(1 hunks)src/dbus/pool/pool_3_9/mod.rs(1 hunks)src/dbus/types.rs(1 hunks)src/dbus/util.rs(1 hunks)src/engine/types/keys.rs(4 hunks)src/engine/types/mod.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/dbus/pool/mod.rs
- src/dbus/pool/pool_3_9/mod.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
Repo: stratis-storage/stratisd PR: 3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/dbus/manager/mod.rssrc/dbus/manager/manager_3_2/methods.rssrc/dbus/manager/manager_3_1/mod.rssrc/engine/types/keys.rssrc/dbus/util.rssrc/dbus/manager/manager_3_0/props.rssrc/dbus/types.rssrc/dbus/manager/manager_3_0/methods.rssrc/dbus/manager/manager_3_0/mod.rssrc/dbus/manager/manager_3_2/mod.rs
📚 Learning: 2025-09-05T14:32:42.528Z
Learnt from: jbaublitz
Repo: stratis-storage/stratisd PR: 3898
File: src/engine/sim_engine/pool.rs:934-948
Timestamp: 2025-09-05T14:32:42.528Z
Learning: In src/engine/sim_engine/pool.rs, the start_encrypt_pool method intentionally creates a SizedKeyMemory with 1 byte allocated but 0 length reported - this is to avoid NULL pointer issues while simulating an empty key slice in the sim engine.
Applied to files:
src/dbus/manager/manager_3_0/methods.rs
🧬 Code graph analysis (13)
src/dbus/filesystem/mod.rs (3)
src/dbus/pool/pool_3_9/mod.rs (2)
connection(73-73)register(49-67)src/dbus/manager/manager_3_0/mod.rs (1)
register(53-70)src/dbus/filesystem/filesystem_3_9/mod.rs (1)
register(30-41)
src/dbus/manager/mod.rs (5)
src/dbus/pool/pool_3_9/mod.rs (2)
connection(73-73)register(49-67)src/dbus/manager/manager_3_2/mod.rs (1)
register(61-78)src/dbus/manager/manager_3_1/mod.rs (1)
register(51-68)src/dbus/manager/manager_3_0/mod.rs (1)
register(53-70)src/dbus/manager/manager_3_9/mod.rs (1)
register(43-60)
src/dbus/manager/manager_3_2/methods.rs (3)
src/dbus/filesystem/mod.rs (1)
register_filesystem(25-43)src/dbus/pool/mod.rs (1)
unregister_pool(53-67)src/dbus/util.rs (9)
engine_to_dbus_err_tuple(67-73)send_locked_pools_signals(123-145)send_stopped_pools_signals(147-160)tuple_to_option(27-33)connection(84-86)connection(97-99)connection(126-128)connection(135-137)connection(150-152)
src/dbus/mod.rs (4)
src/dbus/util.rs (7)
connection(84-86)connection(97-99)connection(126-128)connection(135-137)connection(150-152)send_fs_background_signals(115-121)send_pool_background_signals(76-112)src/dbus/manager/mod.rs (1)
register_manager(112-127)src/dbus/udev.rs (1)
new(25-39)src/engine/structures/lock.rs (1)
new_shared(84-86)
src/dbus/manager/manager_3_1/mod.rs (2)
src/dbus/manager/manager_3_0/props.rs (1)
version_prop(12-14)src/dbus/util.rs (5)
connection(84-86)connection(97-99)connection(126-128)connection(135-137)connection(150-152)
src/engine/types/keys.rs (2)
src/engine/strat_engine/liminal/device_info.rs (11)
into(74-101)into(194-221)into(268-273)into(773-775)into(804-806)from(61-68)from(125-133)from(182-188)from(257-262)from(472-480)from(484-492)src/dbus/types.rs (2)
from(47-110)from(204-210)
src/dbus/util.rs (2)
src/stratis/errors.rs (9)
fmt(93-159)from(165-167)from(171-173)from(177-179)from(183-185)from(189-191)from(195-197)from(201-203)from(207-209)src/dbus/types.rs (2)
from(47-110)from(204-210)
src/dbus/manager/manager_3_0/props.rs (1)
src/stratis/keys.rs (1)
engine(28-46)
src/dbus/types.rs (1)
src/dbus/util.rs (1)
result_option_to_tuple(50-63)
src/dbus/manager/manager_3_0/methods.rs (3)
src/dbus/pool/mod.rs (2)
register_pool(26-51)unregister_pool(53-67)src/dbus/util.rs (8)
engine_to_dbus_err_tuple(67-73)send_locked_pools_signals(123-145)tuple_to_option(27-33)connection(84-86)connection(97-99)connection(126-128)connection(135-137)connection(150-152)src/engine/types/keys.rs (13)
new(46-48)new(126-144)new(278-282)new(728-730)key_desc(84-89)clevis_info(91-96)clevis_info(628-641)from(251-258)from(645-661)from(669-677)from(813-820)new_legacy(105-124)uuid(732-734)
src/engine/types/mod.rs (2)
src/dbus/pool/pool_3_9/mod.rs (2)
Uuid(98-100)Name(104-106)src/dbus/types.rs (1)
from(47-110)
src/dbus/manager/manager_3_0/mod.rs (3)
src/dbus/manager/manager_3_0/methods.rs (7)
create_pool_method(99-157)destroy_pool_method(159-206)list_keys_method(34-44)set_key_method(46-74)unlock_pool_method(208-236)unset_key_method(76-96)devs(111-111)src/dbus/manager/manager_3_0/props.rs (2)
locked_pools_prop(16-18)version_prop(12-14)src/dbus/manager/manager_3_9/mod.rs (2)
new(29-41)register(43-60)
src/dbus/manager/manager_3_2/mod.rs (3)
src/dbus/manager/manager_3_0/props.rs (1)
version_prop(12-14)src/dbus/manager/manager_3_2/methods.rs (3)
refresh_state_method(202-207)start_pool_method(28-129)stop_pool_method(131-200)src/dbus/manager/manager_3_2/props.rs (1)
stopped_pools_prop(12-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (33)
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: checks-with-ci-repo
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile build, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile build-min, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile test, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.91.0, clippy)
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.91.0, cargo)
- GitHub Check: dummy
- GitHub Check: checks (make -f Makefile build-min, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.91.0, clippy)
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.91.0, cargo)
- GitHub Check: tests-with-testing-repo (master)
- GitHub Check: python-based-tests
- GitHub Check: shell-checks
- GitHub Check: checks (make -f Makefile build, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile test, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile fmt-ci, 1.91.0, rustfmt)
🔇 Additional comments (15)
src/dbus/manager/manager_3_0/props.rs (1)
12-18: LGTM!The property functions are straightforward wrappers that correctly delegate to the version constant and engine method.
src/engine/types/mod.rs (3)
109-109: LGTM!The change from
zbus::zvariant::Valuetoserde_json::Valuefor ClevisInfo aligns with the serialization patterns used elsewhere in the D-Bus integration.
58-105: LGTM!The conditional compilation pattern correctly adds zbus Type and Value support for UUIDs when the dbus_enabled feature is active.
168-175: LGTM!The conditional Type and Value derives for the Name type are correctly gated by the dbus_enabled feature.
src/dbus/manager/manager_3_1/mod.rs (1)
29-141: LGTM!The ManagerR1 implementation follows the established pattern from ManagerR0 and correctly delegates to shared method implementations.
src/dbus/manager/manager_3_0/mod.rs (1)
31-143: LGTM!The ManagerR0 implementation establishes the base pattern and correctly delegates all method implementations to the shared manager_3_0/methods module.
src/dbus/manager/manager_3_2/props.rs (1)
12-16: LGTM!The stopped_pools_prop function correctly wraps the engine result in the ManagerR2 type.
src/dbus/manager/mod.rs (1)
112-127: LGTM!The register_manager function correctly registers all manager interface versions and the ObjectManager.
src/dbus/filesystem/mod.rs (1)
25-43: LGTM!The filesystem registration correctly sequences object registration before manager state updates, ensuring consistency.
src/dbus/mod.rs (1)
32-53: LGTM!The create_dbus_handler function correctly initializes and wires up all D-Bus components in the proper sequence.
src/dbus/manager/manager_3_2/methods.rs (1)
202-207:refresh_state_methodwrapper is straightforwardThinly forwarding to
engine.refresh_state().awaitand reusingDbusErrorEnum/engine_to_dbus_err_tuplekeeps the DBus contract consistent with other methods. No issues spotted here.src/dbus/types.rs (2)
46-111: Locked pools conversion to DBusDictlooks consistent with the declared signatureThe
Typeimpl forLockedPoolsInfo(outerDict<str, Dict<str, variant>>) matches howValue::from(LockedPoolsInfo)populatestop_level_dictand the inner dict (key_description,clevis_info,devs,name), and the use ofresult_option_to_tuplefor the optional fields keeps the on‑wire form uniform.No functional issues stand out here.
185-216:ManagerR2<StoppedPoolsInfo>andPoolUuidDBus typing look good
- The
Typeimpl forManagerR2<StoppedPoolsInfo>reuses the same nestedDict<str, Dict<str, variant>>signature asLockedPoolsInfo, andFrom<ManagerR2<StoppedPoolsInfo>> for Valuedelegates cleanly viastopped_pools_to_value.- The
Basicimpl forPoolUuidas a string ('s') is consistent with using UUID strings as keys (e.g., intop_level_dict.add(*uuid, Value::Dict(dict))).No changes needed here.
src/dbus/manager/manager_3_2/mod.rs (2)
39-79: ManagerR2 struct andregisterwiring look correctThe
ManagerR2struct cleanly captures the sharedEngine,Connection,Managerlock, and counter, andregister():
- Clones the Arcs/Lockable exactly once, and
- Registers the instance at
consts::STRATIS_BASE_PATHviaobject_server().at(...).This matches the pattern used for other manager versions and should integrate smoothly with the new zbus-based object server.
81-175: Manager.r2 interface methods correctly delegate to existing helpersThe DBus interface:
- Reuses
version_propand the 3.0 key/pool helpers, avoiding duplication.- Exposes
StoppedPoolsastypes::ManagerR2<StoppedPoolsInfo>, consistent with theType/Valueimpl insrc/dbus/types.rs.- Routes
StartPool,StopPool, andRefreshStatethrough the 3.2 helpers, preserving a single point of behavior.- Keeps
_redundancyinCreatePoolto maintain the DBus signature, while intentionally ignoring it.The method signatures and return shapes line up with the helper functions and DbusErrorEnum usage; no functional problems seen.
jbaublitz
force-pushed
the
issue-stratisd-2574
branch
from
f599176 to
29b04ad
Compare
jbaublitz
force-pushed
the
issue-stratisd-2574
branch
from
662832e to
5ab9457
Compare
2 participants
Closes #2574
Summary by CodeRabbit
New Features
User-visible Operations
Dependencies
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.