Clean up for registration and deregistration code

[jbaublitz]

Closes #4028

I manually tested with dbus-monitor and did a little bit of additional cleanup to make registration and deregistration consistent.

Summary by CodeRabbit

• Refactor
  • Consolidated pool registration and unregistration into single operations that return and accept pool, filesystem, and device paths.
  • Pool start/create now relies on centralized registration; stop/destroy delegate cleanup to a single unregister flow.
  • Improved robustness: component-unregister failures are logged as warnings rather than aborting the operation.

[packit-as-a-service]

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

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

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

[mulkieran]

@jbaublitz Looks like the format test failure is real, could you fix? The clippy failure is real too.

[mulkieran]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

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

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: aba99b43-c659-4889-a4fa-162c5df09967

📥 Commits

Reviewing files that changed from the base of the PR and between 4403b5c and ac44d5e.

📒 Files selected for processing (7)

• src/dbus/manager/manager_3_0/methods.rs
• src/dbus/manager/manager_3_2/methods.rs
• src/dbus/manager/manager_3_4/methods.rs
• src/dbus/manager/manager_3_6/methods.rs
• src/dbus/manager/manager_3_8/methods.rs
• src/dbus/manager/manager_3_9/methods.rs
• src/dbus/pool/mod.rs

🚧 Files skipped from review as they are similar to previous changes (6)

• src/dbus/manager/manager_3_6/methods.rs
• src/dbus/manager/manager_3_9/methods.rs
• src/dbus/manager/manager_3_4/methods.rs
• src/dbus/pool/mod.rs
• src/dbus/manager/manager_3_0/methods.rs
• src/dbus/manager/manager_3_8/methods.rs

---

Walkthrough

This PR centralizes D-Bus pool component registration and cleanup: register_pool now returns filesystem and blockdev object paths; unregister_pool accepts filesystem and blockdev UUID lists and unregisters components. Manager methods (3.0–3.9) remove per-component register/unregister loops and delegate to these pool functions.

Changes

Pool registration and unregistration centralization

Layer / File(s)	Summary
Core pool module API refactor src/dbus/pool/mod.rs	register_pool now returns (pool_path, fs_paths, bd_paths) instead of (pool_path, bd_paths), accumulating filesystem registration results. unregister_pool accepts explicit fs_uuids and dev_uuids parameters and unregisters each corresponding D-Bus object before unregistering the pool, logging per-item failures as warnings.
Manager 3.0 method updates src/dbus/manager/manager_3_0/methods.rs	create_pool_method now returns blockdev paths from register_pool's third element. destroy_pool_method delegates cleanup to unregister_pool with filesystem and blockdev UUID lists. Imports updated to remove unregister_blockdev and unregister_filesystem.
Manager 3.2 method updates src/dbus/manager/manager_3_2/methods.rs	start_pool_method obtains pool, filesystem, and blockdev paths from a single register_pool call instead of manual filesystem registration. stop_pool_method delegates cleanup to unregister_pool with explicit UUID lists instead of computing and individually unregistering components.
Manager 3.4 method updates src/dbus/manager/manager_3_4/methods.rs	start_pool_method removes the manual register_filesystem loop and obtains filesystem paths directly from register_pool. Imports updated to remove register_filesystem dependency.
Manager 3.6 method updates src/dbus/manager/manager_3_6/methods.rs	stop_pool_method removes the logic that computed remaining UUID sets and individually unregistered components. Cleanup delegated to unregister_pool. Imports updated to remove HashSet usage.
Manager 3.8 method updates src/dbus/manager/manager_3_8/methods.rs	create_pool_method destructures blockdev paths from register_pool's return tuple. start_pool_method removes manual filesystem registration loop and obtains paths directly from register_pool. Imports updated to remove register_filesystem dependency.
Manager 3.9 method updates src/dbus/manager/manager_3_9/methods.rs	start_pool_method removes manual register_filesystem loop and uses filesystem and blockdev paths directly from register_pool. Return construction simplified to use dev_paths directly. Imports updated to remove register_filesystem dependency.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

• mulkieran

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Clean up for registration and deregistration code' accurately describes the primary change across all modified files, which refactor registration and deregistration logic.
Linked Issues check	✅ Passed	The changes address issue #4028 by refactoring registration/deregistration code to ensure the Pool property is included in filesystem announcements through consolidated pool registration logic.
Out of Scope Changes check	✅ Passed	All code changes are directly related to the registration and deregistration refactoring objective, with no unrelated or out-of-scope modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

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

114-121: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Remove redundant conversion and fix formatting to pass CI.

Two pipeline failures on this block:

1. Clippy error (Line 118): OwnedObjectPath::from(pool_path) is a useless conversion since pool_path is already OwnedObjectPath (assigned at line 99).
2. Rustfmt (Line 114): The tuple formatting should be compacted.

🐛 Proposed fix

             (
-                (
-                    true,
-                    (
-                        OwnedObjectPath::from(pool_path),
-                        dev_paths,
-                        fs_paths,
-                    ),
-                ),
+                (true, (pool_path, dev_paths, fs_paths)),
                 DbusErrorEnum::OK as u16,
                 OK_STRING.to_string(),
             )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/dbus/manager/manager_3_9/methods.rs` around lines 114 - 121, Remove the
redundant OwnedObjectPath::from(pool_path) conversion and compact the tuple
formatting: replace the tuple arm that currently uses
OwnedObjectPath::from(pool_path) with just pool_path (since pool_path is already
an OwnedObjectPath) and reformat the tuple to a single-line/compact form
containing (true, (pool_path, dev_paths, fs_paths)) so Clippy and rustfmt pass;
look for this pattern in methods.rs around the tuple containing pool_path,
dev_paths, and fs_paths.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/dbus/manager/manager_3_6/methods.rs`:
- Line 62: The line with the awaited call to unregister_pool is misformatted for
rustfmt; reflow the if-let expression so the call arguments or the .await are on
their own line(s) to satisfy rustfmt—for example, break the call across lines:
split the argument list to multiple lines or move .await to a new line while
keeping the pattern if let Err(e) = ... so locate the call to
unregister_pool(connection, manager, &pool.as_ref(), &fs_uuids,
&dev_uuids).await and reflow it into a multi-line call expression (preserving
the same parameters and variable names) so rustfmt passes.

In `@src/dbus/pool/mod.rs`:
- Around line 14-23: This file fails rustfmt; run cargo fmt (or rustfmt) and
reformat src/dbus/pool/mod.rs so the import block and surrounding module code
adhere to rustfmt rules—fix spacing/commas/indentation in the dbus::{
unregister_blockdev, consts, filesystem::unregister_filesystem,
register_blockdev, register_filesystem, Manager }, engine::{ DevUuid, Engine,
FilesystemUuid, Lockable, PoolIdentifier, PoolUuid } and stratis::{
StratisError, StratisResult } groups and any subsequent blocks that reference
PoolUuid/PoolIdentifier so the file passes CI formatting checks, then re-commit.

---

Outside diff comments:
In `@src/dbus/manager/manager_3_9/methods.rs`:
- Around line 114-121: Remove the redundant OwnedObjectPath::from(pool_path)
conversion and compact the tuple formatting: replace the tuple arm that
currently uses OwnedObjectPath::from(pool_path) with just pool_path (since
pool_path is already an OwnedObjectPath) and reformat the tuple to a
single-line/compact form containing (true, (pool_path, dev_paths, fs_paths)) so
Clippy and rustfmt pass; look for this pattern in methods.rs around the tuple
containing pool_path, dev_paths, and fs_paths.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 489a35c2-7529-4b97-9192-07693121f691

📥 Commits

Reviewing files that changed from the base of the PR and between 3f42254 and 4403b5c.

📒 Files selected for processing (7)

• src/dbus/manager/manager_3_0/methods.rs
• src/dbus/manager/manager_3_2/methods.rs
• src/dbus/manager/manager_3_4/methods.rs
• src/dbus/manager/manager_3_6/methods.rs
• src/dbus/manager/manager_3_8/methods.rs
• src/dbus/manager/manager_3_9/methods.rs
• src/dbus/pool/mod.rs

[mulkieran]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

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

[mulkieran]

I'm not done review yet, but I wanted to note one significant concern in the comment.

[mulkieran]

The fs_uuids_rem values in the removed code address a particular scenario where a pool is partially stopped; the pool begins tearing down the filesystem devices but then encounters a situation where the filesystem is still mounted, and stops. So the pool is partially torn down, but filesystems are still remaining and the pool should not be unregistered. I don't think it can remove, but I also think that combining the action taken on Stopped and Partial results is probably mistaken and should also be fixed. The same objection holds for the r6 implementation.