Rewrite stratis-decode-dm in Rust

[mulkieran]

Closes #3976

[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]

Walkthrough

This PR rewrites the stratis-decode-dm utility from Python to Rust, integrating D-Bus communication capabilities through new dependencies. Associated CI pipeline changes remove Python linting infrastructure and update build configurations to support the new client_dbus_enabled feature flag.

Changes

Cohort / File(s)	Summary
CI & Linting Infrastructure .githooks/pre-commit, .github/workflows/main.yml, Makefile, .packit.yaml	Removed Python lint steps from pre-commit hook and GitHub Actions workflow; deleted lint target from Makefile; updated Packit CI repository URL and branch for issue_stratisd_3976.
Cargo Configuration Cargo.toml	Added new client_dbus_enabled feature alias wiring zbus and zbus_names dependencies; introduced zbus_names 4.2.0 as optional dependency; enabled derive feature for Clap; updated stratis-utils binary to require both engine and client_dbus_enabled features.
stratis-decode-dm Rust Migration src/bin/utils/decode_dm.rs, src/bin/utils/cmds.rs, src/bin/utils/mod.rs, src/bin/utils/stratis-decode-dm	Implemented new decode_dm Rust module (~203 lines) providing D-Bus integration to resolve device-mapper paths to pool/filesystem names and symlinks; added StratisDecodeDm command with OutputMode enum to cmds.rs; declared new decode_dm module in mod.rs; removed equivalent Python script.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	Changes include scope creep beyond the Rust rewrite: removal of Python linting from CI/pre-commit, updates to Makefile removing Python formatting steps, and packit.yaml CI URL changes are unrelated to the rewrite itself.	Isolate the Rust rewrite changes from CI/linting infrastructure changes and publish them as separate pull requests for clearer review and potential reversal.
Docstring Coverage	⚠️ Warning	Docstring coverage is 26.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: rewriting the stratis-decode-dm tool from Python to Rust.
Linked Issues check	✅ Passed	The PR successfully rewrites stratis-decode-dm in Rust as required by issue #3976, implementing all three public functions (pool_name, filesystem_name, symlink) with D-Bus integration.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

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

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

---

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: 1

🧹 Nitpick comments (2)

src/bin/utils/decode_dm.rs (2)

116-118: Consider more descriptive expect messages.

The expect("Obviously") messages could be more helpful for debugging if these ever fail unexpectedly. A message like "interface name is valid by construction" would be clearer.

Suggested improvement

     let (pool_interface_name, filesystem_interface_name) = (
-        InterfaceName::try_from(POOL_INTERFACE.clone()).expect("Obviously"),
-        InterfaceName::try_from(FILESYSTEM_INTERFACE.clone()).expect("Obviously"),
+        InterfaceName::try_from(POOL_INTERFACE.clone()).expect("pool interface name is valid by construction"),
+        InterfaceName::try_from(FILESYSTEM_INTERFACE.clone()).expect("filesystem interface name is valid by construction"),
     );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/bin/utils/decode_dm.rs` around lines 116 - 118, Replace the placeholder
expect messages on the InterfaceName::try_from calls for POOL_INTERFACE and
FILESYSTEM_INTERFACE with descriptive failure texts; update the two
expect("Obviously") usages to something like "interface name is valid by
construction" or "failed to convert POOL_INTERFACE to InterfaceName" / "failed
to convert FILESYSTEM_INTERFACE to InterfaceName" so any panic includes which
constant conversion failed (refer to InterfaceName::try_from, POOL_INTERFACE,
FILESYSTEM_INTERFACE).

---

19-27: Consider documenting or externalizing REVISION_NUMBER.

The revision number 9 is hardcoded and determines the D-Bus interface version (e.g., org.storage.stratis3.pool.r9). If the interface revision changes, this will need manual updating. Consider adding a comment explaining how this value relates to stratisd's D-Bus interface versioning, or whether it should be shared with other parts of the codebase.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/bin/utils/decode_dm.rs` around lines 19 - 27, REVISION_NUMBER is
hardcoded (9) and drives the generated D-Bus interface strings (REVISION,
POOL_INTERFACE, FILESYSTEM_INTERFACE); add a short doc comment above
REVISION_NUMBER explaining that it maps to stratisd's D-Bus interface revision
and how/where it should be updated, or refactor to obtain the value from a
shared constant or configuration (env var/const in a common module) so multiple
modules share the same source of truth; ensure the comment references stratisd
D-Bus versioning and update usages of REVISION, POOL_INTERFACE, and
FILESYSTEM_INTERFACE to rely on the single documented/shared source.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.packit.yaml:
- Line 13: Replace the temporary fork reference in .packit.yaml: change the git
clone URL string "git clone https://github.com/mulkieran/ci
--branch=issue_stratisd_3976 --depth=1 ../distro" back to the official upstream
repository (e.g., use "git clone https://github.com/stratis-storage/ci" with the
appropriate branch or default) so the packit configuration points to
stratis-storage/ci instead of mulkieran/ci before merging.

---

Nitpick comments:
In `@src/bin/utils/decode_dm.rs`:
- Around line 116-118: Replace the placeholder expect messages on the
InterfaceName::try_from calls for POOL_INTERFACE and FILESYSTEM_INTERFACE with
descriptive failure texts; update the two expect("Obviously") usages to
something like "interface name is valid by construction" or "failed to convert
POOL_INTERFACE to InterfaceName" / "failed to convert FILESYSTEM_INTERFACE to
InterfaceName" so any panic includes which constant conversion failed (refer to
InterfaceName::try_from, POOL_INTERFACE, FILESYSTEM_INTERFACE).
- Around line 19-27: REVISION_NUMBER is hardcoded (9) and drives the generated
D-Bus interface strings (REVISION, POOL_INTERFACE, FILESYSTEM_INTERFACE); add a
short doc comment above REVISION_NUMBER explaining that it maps to stratisd's
D-Bus interface revision and how/where it should be updated, or refactor to
obtain the value from a shared constant or configuration (env var/const in a
common module) so multiple modules share the same source of truth; ensure the
comment references stratisd D-Bus versioning and update usages of REVISION,
POOL_INTERFACE, and FILESYSTEM_INTERFACE to rely on the single documented/shared
source.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7bbe363 and c671b3c.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (9)

• .githooks/pre-commit
• .github/workflows/main.yml
• .packit.yaml
• Cargo.toml
• Makefile
• src/bin/utils/cmds.rs
• src/bin/utils/decode_dm.rs
• src/bin/utils/mod.rs
• src/bin/utils/stratis-decode-dm

💤 Files with no reviewable changes (3)

• .githooks/pre-commit
• src/bin/utils/stratis-decode-dm
• .github/workflows/main.yml

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Revert to upstream CI repository before merge.

This references a personal fork (mulkieran/ci) with a feature branch instead of the official stratis-storage/ci repository. This appears to be a temporary change for testing (consistent with the "TEMPORARY" commit message). Ensure this is reverted to the upstream repository before merging to master.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.packit.yaml at line 13, Replace the temporary fork reference in
.packit.yaml: change the git clone URL string "git clone
https://github.com/mulkieran/ci --branch=issue_stratisd_3976 --depth=1
../distro" back to the official upstream repository (e.g., use "git clone
https://github.com/stratis-storage/ci" with the appropriate branch or default)
so the packit configuration points to stratis-storage/ci instead of mulkieran/ci
before merging.

[mulkieran]

Build failures look like it's because I'm depending on an enhancement of zbus_names in 4.3.0 that is not available in 4.2.0 which is what there is in Fedora.

[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-3977-copr_pull
• And now you can install the packages.

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

[mulkieran]

there was a bug in the testing repo test, variable shadowed. should be fixed now.

[mulkieran]

Rebased and removed top-level .isort.cfg.

[mulkieran]

rebased.

[mulkieran]

rebased.

[mulkieran]

This time it must succeed.

[mulkieran]

rebased.

[jbaublitz]

Just a couple of comments but otherwise this looks good.

[jbaublitz]

I think we can remove this altogether by simply changing the format String in POOL_INTERFACE and FILESYSTEM_INTERFACE.

[mulkieran]

@coderabbitai, how would I implement the reviewer's suggestion?

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
cat src/bin/utils/decode_dm.rs

Length of output: 7411

---

@mulkieran, the reviewer's suggestion is to remove the intermediate REVISION static entirely and fold the "r" prefix directly into the two interface format strings. Here's the diff:

-static REVISION: LazyLock<String> = LazyLock::new(|| format!("r{REVISION_NUMBER}"));
 static POOL_INTERFACE: LazyLock<String> =
-    LazyLock::new(|| format!("{BUS_NAME}.pool.{}", &*REVISION));
+    LazyLock::new(|| format!("{BUS_NAME}.pool.r{REVISION_NUMBER}"));
 static FILESYSTEM_INTERFACE: LazyLock<String> =
-    LazyLock::new(|| format!("{BUS_NAME}.filesystem.{}", &*REVISION));
+    LazyLock::new(|| format!("{BUS_NAME}.filesystem.r{REVISION_NUMBER}"));

REVISION_NUMBER (the const) can stay since it's still a named, self-documenting source for the version string. After this change the full constants block would look like:

const REVISION_NUMBER: &str = env!("CARGO_PKG_VERSION_MINOR");
const BUS_NAME: &str = "org.storage.stratis3";
const TOP_OBJECT: &str = "/org/storage/stratis3";

static POOL_INTERFACE: LazyLock<String> =
    LazyLock::new(|| format!("{BUS_NAME}.pool.r{REVISION_NUMBER}"));
static FILESYSTEM_INTERFACE: LazyLock<String> =
    LazyLock::new(|| format!("{BUS_NAME}.filesystem.r{REVISION_NUMBER}"));

This removes one LazyLock allocation, eliminates the &*REVISION forced deref, and keeps the logic equally readable.

[mulkieran]

Ok! That was also copied from the Python script directly, but it's also fine this way, done.

[coderabbitai]

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

[bmr-cymru]

Sorry for the lack of comment from me lately. I will add a sprint task to review/test this. My time has been under a great deal of pressure recently & this has slipped off the plate a couple times.

[mulkieran]

review comments addressed, squashed a bit, rebased.