Skip to content

lvm-dbus: bd_lvm_vdo_pool_convert implementation#1166

Open
vojtechtrefny wants to merge 1 commit intostoraged-project:masterfrom
vojtechtrefny:master_lvm-dbus-vdo-convert
Open

lvm-dbus: bd_lvm_vdo_pool_convert implementation#1166
vojtechtrefny wants to merge 1 commit intostoraged-project:masterfrom
vojtechtrefny:master_lvm-dbus-vdo-convert

Conversation

@vojtechtrefny
Copy link
Member

@vojtechtrefny vojtechtrefny commented Feb 6, 2026

Support for LVM VDO pool convert has been added to the DBus API so we can now support it in the DBus plugin too.

Summary by CodeRabbit

  • New Features

    • VDO pool conversion now performs a full DBus-driven creation flow with parameter propagation, guarded configuration updates, and robust error handling.
  • Tests

    • Adjusted test expectations to account for DBus behavior and removed an unimplemented DBus-specific test case.

@coderabbitai
Copy link

coderabbitai bot commented Feb 6, 2026

📝 Walkthrough

Walkthrough

Replaced the stub bd_lvm_vdo_pool_convert with a DBus-backed implementation that builds positional and dictionary parameters, resolves VDO pool LV object paths, sets write policy, updates LVM config under a mutex, calls CreateVdoPool over DBus, and restores config; tests adjusted to account for lvmdbusd behavior and a slow test removed.

Changes

Cohort / File(s) Summary
VDO Pool Conversion Implementation
src/plugins/lvm/lvm-dbus.c
Implemented bd_lvm_vdo_pool_convert: resolve VDO pool LV object path, validate/write-policy, construct tuple+dict DBus parameters, mutex-guard LVM config updates, invoke CreateVdoPool via DBus, handle errors and restore previous config.
Test Adjustments
tests/_lvm_cases.py, tests/lvm_dbus_tests.py
In tests/_lvm_cases.py made lv_info.pool_lv == "testLV" assertion conditional (skipped for dbus tests due to lvmdbusd bug). In tests/lvm_dbus_tests.py removed the test_vdo_pool_convert slow test method.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title clearly and specifically describes the main change: implementing the bd_lvm_vdo_pool_convert function in the lvm-dbus module. This matches the core objective of adding DBus support for LVM VDO pool conversion.

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

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

❤️ Share

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/plugins/lvm/lvm-dbus.c`:
- Around line 4044-4097: The local variable old_config currently has a
g_autofree annotation which causes it to be freed at scope exit and leads to a
use-after-free when you restore global_config_str = old_config; remove
g_autofree from the old_config declaration (make it a plain gchar *old_config =
NULL;) so the manual g_free(global_config_str) and restore logic remains valid;
ensure no other code later frees old_config after restoration to avoid leaks or
double-free.

Support for LVM VDO pool convert has been added to the DBus API so
we can now support it in the DBus plugin too.
@vojtechtrefny vojtechtrefny force-pushed the master_lvm-dbus-vdo-convert branch from c25469f to ef7489a Compare February 6, 2026 12:49
@vojtechtrefny
Copy link
Member Author

Jenkins, test this please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants