Switch to JSON-based output parsing for LVM commands #1182
Merged
vojtechtrefny merged 3 commits intostoraged-project:masterfrom Mar 19, 2026
Merged
Switch to JSON-based output parsing for LVM commands #1182vojtechtrefny merged 3 commits intostoraged-project:masterfrom
vojtechtrefny merged 3 commits intostoraged-project:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (26)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (20)
📝 WalkthroughWalkthroughLVM plugin rewritten to parse JSON reports via json-glib and new tree APIs; build checks/flags for json-glib added. Many plugins now clear cached runtime dependency availability on close; per-filesystem reset hooks added. Dynamic loader uses RTLD_NODELETE. Expanded LVM tests; a plugin-reload test removed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/lvm/lvm-dbus.c (1)
198-220:⚠️ Potential issue | 🟠 MajorAdd NULL safety check to
bd_lvm_closeand properly release the DBus connection object.The function calls
g_dbus_connection_flush_sync()andg_dbus_connection_close_sync()onbuswithout checking if it's NULL, contradicting the comment that acknowledges bus may not be initialized. Additionally,bus = NULLleaks the GObject reference—useg_clear_object(&bus)instead to properly release and NULL the pointer.🔧 Proposed fix
void bd_lvm_close (void) { GError *error = NULL; - /* the check() call should create the DBus connection for us, but let's not - completely rely on it */ - if (!g_dbus_connection_flush_sync (bus, NULL, &error)) { - bd_utils_log_format (BD_UTILS_LOG_CRIT, "Failed to flush DBus connection: %s", error->message); - g_clear_error (&error); - } - if (!g_dbus_connection_close_sync (bus, NULL, &error)) { - bd_utils_log_format (BD_UTILS_LOG_CRIT, "Failed to close DBus connection: %s", error->message); - g_clear_error (&error); + if (bus) { + if (!g_dbus_connection_flush_sync (bus, NULL, &error)) { + bd_utils_log_format (BD_UTILS_LOG_CRIT, "Failed to flush DBus connection: %s", error->message); + g_clear_error (&error); + } + if (!g_dbus_connection_close_sync (bus, NULL, &error)) { + bd_utils_log_format (BD_UTILS_LOG_CRIT, "Failed to close DBus connection: %s", error->message); + g_clear_error (&error); + } + g_clear_object (&bus); } - - bus = NULL; dm_log_with_errno_init (NULL); dm_log_init_verbose (0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/lvm/lvm-dbus.c` around lines 198 - 220, bd_lvm_close calls g_dbus_connection_flush_sync and g_dbus_connection_close_sync on bus without checking for NULL and then sets bus = NULL which leaks the GObject; update bd_lvm_close to first check if (bus != NULL) before calling g_dbus_connection_flush_sync and g_dbus_connection_close_sync, handle and clear GError as already done, and replace the manual bus = NULL with g_clear_object(&bus) to properly unref and NULL the DBus connection object; keep the rest of the cleanup (dm_log_* and avail_* resets) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/btrfs.c`:
- Around line 152-154: bd_btrfs_close currently resets avail_deps and
avail_module_deps with plain assignments which can race with
check_deps()/check_module_deps that use g_atomic_int_get()/g_atomic_int_or();
replace the direct assignments in bd_btrfs_close() with atomic resets using
g_atomic_int_set(&avail_deps, 0) and g_atomic_int_set(&avail_module_deps, 0) so
the cache variables are cleared atomically and avoid race conditions with the
existing atomic readers/updaters.
In `@src/plugins/fs/f2fs.c`:
- Around line 31-35: The current _fs_f2fs_reset_avail_deps function resets
shared flags with plain stores (avail_deps and avail_shrink_deps) which races
with atomic readers/writers in check_deps(); replace the plain assignments with
atomic stores by calling g_atomic_int_set(&avail_deps, 0) and
g_atomic_int_set(&avail_shrink_deps, 0) so the updates are atomic; apply the
same change pattern to the other helper functions named _fs_*_reset_avail_deps
(ext, xfs, vfat, ntfs, exfat, btrfs, udf, nilfs) to ensure all access to those
variables uses g_atomic_* consistently.
In `@src/plugins/lvm/lvm.c`:
- Around line 560-565: The code parses a PE offset into
data->segs[0]->pv_start_pe using atoi(), which is 32-bit and can truncate;
replace the atoi() call with g_ascii_strtoull() to parse a 64-bit value (use
g_ascii_strtoull(paren + 1, NULL, 10)) so the result matches the guint64
pv_start_pe type; keep the existing dev/paren logic
(g_strdup(json_array_get_string_element(...)) and the null-termination of
*paren) unchanged.
- Around line 1799-1817: The dedupe/merge of LV rows currently compares lv_name
which is only unique per VG; update both bd_lvm_lvs and bd_lvm_lvs_tree to use
lv_uuid (the existing UUID field) for duplicate detection/merge instead of
lv_name (replace comparisons and log messages to reference lv_uuid), and ensure
g_ptr_array_index handling uses the lv_uuid member of BDLVMLVdata; additionally
change pv_start_pe parsing (where atoi() is used) to use g_ascii_strtoull() and
proper endptr/base handling to populate the guint64 pv_start_pe safely; finally
add a regression test that queries lvs with identical LV names in different VGs
to confirm de-duplication is by UUID not name.
In `@src/plugins/smart/smartmontools.c`:
- Around line 40-43: The close hook _smart_close_plugin currently writes to
avail_deps with a plain assignment causing a data race; replace the direct write
with an atomic reset by calling g_atomic_int_set(&avail_deps, 0) so the state is
updated atomically and is consistent with other accesses (g_atomic_int_get,
g_atomic_int_or) used in check_deps() and related functions.
---
Outside diff comments:
In `@src/plugins/lvm/lvm-dbus.c`:
- Around line 198-220: bd_lvm_close calls g_dbus_connection_flush_sync and
g_dbus_connection_close_sync on bus without checking for NULL and then sets bus
= NULL which leaks the GObject; update bd_lvm_close to first check if (bus !=
NULL) before calling g_dbus_connection_flush_sync and
g_dbus_connection_close_sync, handle and clear GError as already done, and
replace the manual bus = NULL with g_clear_object(&bus) to properly unref and
NULL the DBus connection object; keep the rest of the cleanup (dm_log_* and
avail_* resets) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee2b2e2c-1edc-4f1b-8655-4c689f35de94
📒 Files selected for processing (30)
configure.acscripts/boilerplate_generator.pysrc/plugins/btrfs.csrc/plugins/dm.csrc/plugins/fs.csrc/plugins/fs/btrfs.csrc/plugins/fs/common.hsrc/plugins/fs/exfat.csrc/plugins/fs/ext.csrc/plugins/fs/f2fs.csrc/plugins/fs/nilfs.csrc/plugins/fs/ntfs.csrc/plugins/fs/udf.csrc/plugins/fs/vfat.csrc/plugins/fs/xfs.csrc/plugins/lvm/Makefile.amsrc/plugins/lvm/lvm-dbus.csrc/plugins/lvm/lvm-private.hsrc/plugins/lvm/lvm.csrc/plugins/mdraid.csrc/plugins/mpath.csrc/plugins/nvdimm.csrc/plugins/s390.csrc/plugins/smart/libatasmart.csrc/plugins/smart/smart-common.csrc/plugins/smart/smart-private.hsrc/plugins/smart/smartmontools.csrc/plugins/swap.ctests/_lvm_cases.pytests/library_test.py
💤 Files with no reviewable changes (1)
- tests/library_test.py
vojtechtrefny
force-pushed
the
master_lvm-json
branch
2 times, most recently
from
46a84eb to
3bdaf7a
Compare
Member
Author
|
@coderabbitai reviewv |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/plugins/lvm/lvm.c (1)
1791-1798:⚠️ Potential issue | 🔴 CriticalUse
uuidfor LV dedupe/merge.Line 1792 and Line 1860 compare rows by
lv_name. That is only unique within a VG, sobd_lvm_lvs(NULL)can drop one LV andbd_lvm_lvs_tree(NULL)can merge segments from different VGs when they reuse the same name.lv_uuidis already in the report and globally unique, so these comparisons should useuuidinstead, with a regression test that creates identical LV names in two VGs.🔧 Suggested fix
- if (g_strcmp0 (((BDLVMLVdata *) g_ptr_array_index (lvs, j))->lv_name, lvdata->lv_name) == 0) { + if (g_strcmp0 (((BDLVMLVdata *) g_ptr_array_index (lvs, j))->uuid, lvdata->uuid) == 0) { @@ - if (g_strcmp0 (other->lv_name, lvdata->lv_name) == 0) { + if (g_strcmp0 (other->uuid, lvdata->uuid) == 0) {Also applies to: 1858-1864
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/lvm/lvm.c` around lines 1791 - 1798, The dedup/merge logic is comparing LV rows by lv_name which is only VG-unique; change those comparisons to use the LV UUID field (lv_uuid) instead: locate the comparison in the loop that inspects g_ptr_array_index(lvs, j) against lvdata (the block that logs "Duplicate LV entry..." and sets duplicate/ calls bd_lvm_lvdata_free) and the similar comparison later around the bd_lvm_lvs_tree handling (the other comparison at ~1858-1864), and replace g_strcmp0(...->lv_name, lvdata->lv_name) checks with comparisons against the uuid field (e.g., ->lv_uuid) so dedupe/merge is done by globally-unique uuid; ensure you still free and mark duplicate exactly as before and add/adjust a regression test that creates identical lv_name in two VGs to verify behavior.
🧹 Nitpick comments (1)
scripts/boilerplate_generator.py (1)
201-201: AddingRTLD_NODELETEis the key change that necessitates all theavail_depsreset changes.With
RTLD_NODELETE, the plugin shared library remains mapped in memory even afterdlclose()succeeds. This means static variables likeavail_depspersist across plugin unload/reload cycles. The coordinated changes to reset these counters in each plugin'sclosefunction ensure fresh dependency checks on reload.Consider adding a brief comment in the generated code explaining why
RTLD_NODELETEis used:- ret += ' handle = dlopen(so_name, RTLD_LAZY | RTLD_NODELETE);\n' + ret += ' /* RTLD_NODELETE keeps the library loaded to preserve symbols; close_fn resets state */\n' + ret += ' handle = dlopen(so_name, RTLD_LAZY | RTLD_NODELETE);\n'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/boilerplate_generator.py` at line 201, The generated code now uses RTLD_NODELETE when calling dlopen which causes plugin static state (e.g., avail_deps) to persist across dlclose/reload; update scripts/boilerplate_generator.py so the string built around the dlopen call (the line containing "handle = dlopen(so_name, RTLD_LAZY | RTLD_NODELETE);") includes a brief inline comment explaining why RTLD_NODELETE is used and that static variables persist across unloads and therefore plugins must reset avail_deps in their close functions; place the comment immediately adjacent to that dlopen line in the generated output so maintainers see the rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/plugins/lvm/lvm.c`:
- Around line 1791-1798: The dedup/merge logic is comparing LV rows by lv_name
which is only VG-unique; change those comparisons to use the LV UUID field
(lv_uuid) instead: locate the comparison in the loop that inspects
g_ptr_array_index(lvs, j) against lvdata (the block that logs "Duplicate LV
entry..." and sets duplicate/ calls bd_lvm_lvdata_free) and the similar
comparison later around the bd_lvm_lvs_tree handling (the other comparison at
~1858-1864), and replace g_strcmp0(...->lv_name, lvdata->lv_name) checks with
comparisons against the uuid field (e.g., ->lv_uuid) so dedupe/merge is done by
globally-unique uuid; ensure you still free and mark duplicate exactly as before
and add/adjust a regression test that creates identical lv_name in two VGs to
verify behavior.
---
Nitpick comments:
In `@scripts/boilerplate_generator.py`:
- Line 201: The generated code now uses RTLD_NODELETE when calling dlopen which
causes plugin static state (e.g., avail_deps) to persist across dlclose/reload;
update scripts/boilerplate_generator.py so the string built around the dlopen
call (the line containing "handle = dlopen(so_name, RTLD_LAZY |
RTLD_NODELETE);") includes a brief inline comment explaining why RTLD_NODELETE
is used and that static variables persist across unloads and therefore plugins
must reset avail_deps in their close functions; place the comment immediately
adjacent to that dlopen line in the generated output so maintainers see the
rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a8dfde8-d6ea-4151-831b-d4d5736b6074
📒 Files selected for processing (30)
configure.acscripts/boilerplate_generator.pysrc/plugins/btrfs.csrc/plugins/dm.csrc/plugins/fs.csrc/plugins/fs/btrfs.csrc/plugins/fs/common.hsrc/plugins/fs/exfat.csrc/plugins/fs/ext.csrc/plugins/fs/f2fs.csrc/plugins/fs/nilfs.csrc/plugins/fs/ntfs.csrc/plugins/fs/udf.csrc/plugins/fs/vfat.csrc/plugins/fs/xfs.csrc/plugins/lvm/Makefile.amsrc/plugins/lvm/lvm-dbus.csrc/plugins/lvm/lvm-private.hsrc/plugins/lvm/lvm.csrc/plugins/mdraid.csrc/plugins/mpath.csrc/plugins/nvdimm.csrc/plugins/s390.csrc/plugins/smart/libatasmart.csrc/plugins/smart/smart-common.csrc/plugins/smart/smart-private.hsrc/plugins/smart/smartmontools.csrc/plugins/swap.ctests/_lvm_cases.pytests/library_test.py
💤 Files with no reviewable changes (1)
- tests/library_test.py
🚧 Files skipped from review as they are similar to previous changes (16)
- configure.ac
- src/plugins/lvm/Makefile.am
- src/plugins/mpath.c
- src/plugins/fs/common.h
- src/plugins/fs/btrfs.c
- src/plugins/fs/f2fs.c
- src/plugins/fs.c
- src/plugins/lvm/lvm-dbus.c
- src/plugins/fs/ntfs.c
- src/plugins/lvm/lvm-private.h
- src/plugins/fs/udf.c
- src/plugins/btrfs.c
- src/plugins/dm.c
- src/plugins/fs/vfat.c
- src/plugins/smart/smart-common.c
- src/plugins/smart/libatasmart.c
Switch pvs, pvinfo, vgs, vginfo, lvs, lvinfo, lvs_tree and lvinfo_tree functions to use --reportformat json_std for parsing LVM command output instead of the text-based --nameprefixes format. This also bumps the minimum required LVM version to 2.03.17 which is the version that introduced the json_std report format. Tests for pvinfo, vginfo, lvinfo and lvinfo_tree have been extended to cover more fields and a new test_lvs_tree test case has been added. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
vojtechtrefny
force-pushed
the
master_lvm-json
branch
from
3bdaf7a to
ed0303f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/plugins/lvm/lvm.c (1)
1787-1794:⚠️ Potential issue | 🔴 CriticalDeduplicate/merge LVs by UUID, not
lv_name.At Line 1790 and Line 1858, matching on
lv_namecan incorrectly collapse distinct LVs from different VGs whenvg_name == NULL. This can silently drop or merge unrelated entries.Suggested fix
- if (g_strcmp0 (((BDLVMLVdata *) g_ptr_array_index (lvs, j))->lv_name, lvdata->lv_name) == 0) { + if (g_strcmp0 (((BDLVMLVdata *) g_ptr_array_index (lvs, j))->uuid, lvdata->uuid) == 0) { bd_utils_log_format (BD_UTILS_LOG_DEBUG, - "Duplicate LV entry for '%s' found in lvs output", - lvdata->lv_name); + "Duplicate LV entry for UUID '%s' found in lvs output", + lvdata->uuid); bd_lvm_lvdata_free (lvdata); duplicate = TRUE; break; } @@ - if (g_strcmp0 (other->lv_name, lvdata->lv_name) == 0) { + if (g_strcmp0 (other->uuid, lvdata->uuid) == 0) { merge_lv_data (other, lvdata); bd_lvm_lvdata_free (lvdata); merged = TRUE; break; }Also applies to: 1856-1859
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/lvm/lvm.c` around lines 1787 - 1794, The duplicate-detection currently compares lv_name (see comparisons using ((BDLVMLVdata *) g_ptr_array_index (lvs, j))->lv_name and lvdata->lv_name) which can collapse distinct LVs across VGs when vg_name is NULL; change the check to compare the LV UUID field (e.g., lv_uuid or whichever UUID member exists on BDLVMLVdata) instead of lv_name in both places (the loop at the shown block and the similar comparison around lines 1856–1859), and ensure bd_lvm_lvdata_free is called only when the UUIDs match to avoid dropping unrelated entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/plugins/lvm/lvm.c`:
- Around line 1787-1794: The duplicate-detection currently compares lv_name (see
comparisons using ((BDLVMLVdata *) g_ptr_array_index (lvs, j))->lv_name and
lvdata->lv_name) which can collapse distinct LVs across VGs when vg_name is
NULL; change the check to compare the LV UUID field (e.g., lv_uuid or whichever
UUID member exists on BDLVMLVdata) instead of lv_name in both places (the loop
at the shown block and the similar comparison around lines 1856–1859), and
ensure bd_lvm_lvdata_free is called only when the UUIDs match to avoid dropping
unrelated entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 692600b6-98e3-4c36-a5a7-b4dd04a63623
📒 Files selected for processing (30)
configure.acscripts/boilerplate_generator.pysrc/plugins/btrfs.csrc/plugins/dm.csrc/plugins/fs.csrc/plugins/fs/btrfs.csrc/plugins/fs/common.hsrc/plugins/fs/exfat.csrc/plugins/fs/ext.csrc/plugins/fs/f2fs.csrc/plugins/fs/nilfs.csrc/plugins/fs/ntfs.csrc/plugins/fs/udf.csrc/plugins/fs/vfat.csrc/plugins/fs/xfs.csrc/plugins/lvm/Makefile.amsrc/plugins/lvm/lvm-dbus.csrc/plugins/lvm/lvm-private.hsrc/plugins/lvm/lvm.csrc/plugins/mdraid.csrc/plugins/mpath.csrc/plugins/nvdimm.csrc/plugins/s390.csrc/plugins/smart/libatasmart.csrc/plugins/smart/smart-common.csrc/plugins/smart/smart-private.hsrc/plugins/smart/smartmontools.csrc/plugins/swap.ctests/_lvm_cases.pytests/library_test.py
💤 Files with no reviewable changes (1)
- tests/library_test.py
🚧 Files skipped from review as they are similar to previous changes (17)
- src/plugins/swap.c
- src/plugins/fs/ext.c
- src/plugins/s390.c
- src/plugins/fs/btrfs.c
- src/plugins/fs.c
- src/plugins/fs/common.h
- scripts/boilerplate_generator.py
- src/plugins/nvdimm.c
- src/plugins/fs/xfs.c
- src/plugins/lvm/lvm-dbus.c
- configure.ac
- src/plugins/dm.c
- src/plugins/smart/smart-common.c
- src/plugins/lvm/lvm-private.h
- src/plugins/smart/libatasmart.c
- src/plugins/smart/smart-private.h
- src/plugins/fs/nilfs.c
Member
Author
|
/packit test |
tbzatek
previously approved these changes
Mar 18, 2026
Member
tbzatek
left a comment
tbzatek
left a comment
There was a problem hiding this comment.
Quite heavy, but makes sense. The plugin unloading/deinitialization makes sense, though limiting and suboptimal, but given the requirements I don't have any better idea.
…close Use RTLD_NODELETE flag with dlopen when loading plugins to prevent shared libraries (like json-glib) from being unmapped on dlclose. This avoids GType re-registration failures when a plugin is reloaded via reinit, since GTypes are permanent but the static variables guarding their registration would be lost if the library is unmapped. As a consequence of RTLD_NODELETE keeping libraries mapped, static variables caching dependency check results (avail_deps, avail_features, etc.) now persist across plugin reload cycles. Reset these caches in each plugin's close function so that dependency availability is properly re-evaluated on the next load. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The test_reload test recompiled the LVM plugin and verified that reinit picked up the new code. This no longer works because plugins are loaded with RTLD_NODELETE, which prevents the shared library from being unmapped on dlclose -- dlopen of the same path returns the already-loaded library. The test_force_plugin test already covers reloading with a different .so name. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
vojtechtrefny
force-pushed
the
master_lvm-json
branch
from
ed0303f to
9663e3d
Compare
tbzatek
approved these changes
Mar 19, 2026
vojtechtrefny
merged commit ef1c698
into
storaged-project:master
44 of 48 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #729
Summary by CodeRabbit
Bug Fixes
Improvements
Updated Support
Tests