dm: Use libdevmapper instead of dmsetup to get subsystem from name #1159
Conversation
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (42)
🔇 Additional comments (4)
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: 1
🤖 Fix all issues with AI agents
In @tests/dm_test.py:
- Around line 88-93: After creating the device with
BlockDev.dm_create_linear("testMap", ...) assert success, add a check that
queries the device mapping/subsystem for "testMap" (for example using the
existing helper that reads DM metadata such as BlockDev.dm_get_subsystem or
other test helper that returns the subsystem) and assert that the returned
subsystem equals "TEST" before calling BlockDev.dm_remove("testMap"); this
ensures the UUID-without-hyphen case is validated prior to removal.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/plugin_apis/dm.apisrc/plugins/dm.csrc/plugins/dm.htests/dm_test.py
⏰ 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). (42)
- GitHub Check: testing-farm:fedora-43-aarch64
- GitHub Check: testing-farm:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-43-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-aarch64
- GitHub Check: rpm-build:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-rawhide-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-aarch64
- GitHub Check: rpm-build:fedora-43-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-43-aarch64
- GitHub Check: testing-farm:fedora-43-x86_64:revdeps_udisks
- GitHub Check: testing-farm:fedora-43-x86_64:revdeps_blivet
- GitHub Check: testing-farm:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-rawhide-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-aarch64
- GitHub Check: rpm-build:fedora-43-ppc64le
- GitHub Check: rpm-build:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: compilation (clang-17)
- GitHub Check: compilation (clang-18)
- GitHub Check: compilation (gcc-12)
- GitHub Check: compilation (gcc-10)
- GitHub Check: compilation (clang-16)
- GitHub Check: compilation (clang-14)
- GitHub Check: compilation (gcc-14)
- GitHub Check: compilation (gcc-11)
- GitHub Check: compilation (gcc-13)
- GitHub Check: compilation (clang-15)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
- GitHub Check: csmock
- GitHub Check: symbols
- GitHub Check: blivet-tests
- GitHub Check: udisks-build
🔇 Additional comments (8)
src/lib/plugin_apis/dm.api (1)
20-20: LGTM!The new error code
BD_DM_ERROR_DEVICE_NOEXISTis correctly added at the end of the enum, maintaining ABI compatibility. The naming follows existing conventions in the codebase.src/plugins/dm.h (1)
16-16: LGTM!The enum addition is consistent with the public API declaration in
dm.api.src/plugins/dm.c (2)
21-21: LGTM!The
<string.h>include is correctly added forstrchr()used in the subsystem extraction logic.
239-302: Well-structured refactor to use libdevmapper API.The implementation correctly:
- Checks root privileges upfront
- Manages the DM task lifecycle with proper cleanup on all paths
- Uses the new
BD_DM_ERROR_DEVICE_NOEXISTerror code for non-existent devices- Handles edge cases (NULL/empty UUID returns empty string)
- Extracts subsystem as the UUID prefix before the first hyphen
tests/dm_test.py (4)
95-97: LGTM!Good test coverage for the new
BD_DM_ERROR_DEVICE_NOEXISTerror path.
107-112: LGTM!Good additions to verify filesystem-level side effects of device creation and removal.
154-158: LGTM!The class rename to
DMNoStorageTestbetter reflects its purpose. The plugin version test is a useful sanity check.
71-72: LGTM!Using
--pbkdf=pbkdf2 --pbkdf-force-iterations=1000is a good optimization for faster test execution.
vojtechtrefny
force-pushed
the
master_dm-subsystem-libdevmapper
branch
from
683495a to
3ca96b3
Compare
vojtechtrefny
force-pushed
the
master_dm-subsystem-libdevmapper
branch
2 times, most recently
from
3ca96b3 to
a8a61c0
Compare
Faster pbkdf when creating LUKS device for testing, reducing number of test cases and adding some additional checks.
Checking only EUID is not enough and we don't really support running without root privileges. If someone tries to run this function as non-root, the first dm_task_create will fail and the reason is logged, so that should be good enough.
vojtechtrefny
force-pushed
the
master_dm-subsystem-libdevmapper
branch
from
bfeae65 to
80ba38c
Compare
2 participants
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.