Various fixes for issues discovered by Claude

[vojtechtrefny]

Next batch, this time mostly in the .api files.

Summary by CodeRabbit

Bug Fixes

• Fixed memory management and resource cleanup issues across storage plugins, including proper deallocation of namespace data and cache statistics
• Corrected cache pool conversion logic to ensure volume operations occur only after successful pool creation
• Enhanced initialization and deep copying of storage device data structures
• Improved error handling with proper cleanup of allocated resources and null pointer safety

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR includes bug fixes across plugin APIs and implementations, focusing on memory management (struct field copying, deep copies, cleanup on error paths), zero-initialization of allocated structs, control flow corrections, and enhanced test validation for cache operations.

Changes

Cohort / File(s)	Summary
Plugin API Copy and Cleanup Routines src/lib/plugin_apis/fs.api, src/lib/plugin_apis/lvm.api, src/lib/plugin_apis/mdraid.api, src/lib/plugin_apis/nvdimm.api	Added missing free_cluster_count field copy in VFAT copy routine; corrected bd_lvm_cache_stats_free type parameter and added NULL pointer check; added missing total_devices field copy in MD detail copy routine; changed dev field from pointer copy to deep copy using g_strdup().
Plugin Implementation Memory Management src/plugins/crypto.c, src/plugins/nvdimm.c, src/plugins/part.c	Added cleanup of partially-populated out-parameter and removed crypt_free() call on error path in crypto operations; added proper cleanup for empty namespace array in list operation; removed unnecessary unref of uninitialized partition object.
Plugin Implementation Initialization and Logic src/plugins/btrfs.c, src/plugins/lvm/lvm-dbus.c	Replaced g_new() with g_new0() for zero-initialization across three device/subvolume/filesystem info helpers; corrected condition for bd_lvm_lvrename invocation to occur only on cache pool creation success.
Test Validation tests/_lvm_cases.py	Enhanced test_cache_pool_convert with additional assertions to verify returned cache info object name and attributes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 63.64% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Various fixes for issues discovered by Claude' is too vague and generic. It uses non-descriptive language that doesn't convey what specific issues are being fixed or which components are affected.	Replace with a more specific title that highlights the primary fix category, such as 'Fix memory safety and copy field issues across fs, lvm, mdraid, nvdimm, and other components' or focus on the most impactful change.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ 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: 1

🧹 Nitpick comments (1)

src/plugins/nvdimm.c (1)

567-571: Pre-existing issue: potential memory leak in error path.

The error handling path uses g_ptr_array_free(namespaces, FALSE) after manually freeing elements. When FALSE is passed, g_ptr_array_free returns the underlying element array pointer, but this return value is discarded, potentially leaking that memory.

Consider changing to TRUE for consistency with the fix at line 580:

♻️ Suggested fix

 if (!info) {
     g_ptr_array_foreach (namespaces, (GFunc) (void *) bd_nvdimm_namespace_info_free, NULL);
-    g_ptr_array_free (namespaces, FALSE);
+    g_ptr_array_free (namespaces, TRUE);
     ndctl_unref (ctx);
     return NULL;
 }

🤖 Prompt for AI Agents

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

In `@src/plugins/nvdimm.c` around lines 567 - 571, The error path frees each
element via bd_nvdimm_namespace_info_free and then calls
g_ptr_array_free(namespaces, FALSE) which discards the returned element array
and leaks it; change the call to g_ptr_array_free(namespaces, TRUE) (in the
block that also calls bd_nvdimm_namespace_info_free and ndctl_unref(ctx) and
returns NULL) so the underlying array storage is freed as well, matching the
later fix at the other return path.

🤖 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/lvm/lvm-dbus.c`:
- Around line 3717-3718: The code ignores bd_lvm_lvrename()'s return value
causing bd_lvm_cache_pool_convert() to report success even if the rename failed;
update the call in bd_lvm_cache_pool_convert so you capture and fold the rename
result into the existing status (e.g., set ret = ret && bd_lvm_lvrename(vg_name,
data_lv, name, NULL, error)) so the final return reflects both the
CreateCachePool and the LV rename outcome.

---

Nitpick comments:
In `@src/plugins/nvdimm.c`:
- Around line 567-571: The error path frees each element via
bd_nvdimm_namespace_info_free and then calls g_ptr_array_free(namespaces, FALSE)
which discards the returned element array and leaks it; change the call to
g_ptr_array_free(namespaces, TRUE) (in the block that also calls
bd_nvdimm_namespace_info_free and ndctl_unref(ctx) and returns NULL) so the
underlying array storage is freed as well, matching the later fix at the other
return path.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 92a2b6f7-62de-4b4f-b3c6-7a97c630a5b6

📥 Commits

Reviewing files that changed from the base of the PR and between 37ef834 and 4de29e4.

📒 Files selected for processing (10)

• src/lib/plugin_apis/fs.api
• src/lib/plugin_apis/lvm.api
• src/lib/plugin_apis/mdraid.api
• src/lib/plugin_apis/nvdimm.api
• src/plugins/btrfs.c
• src/plugins/crypto.c
• src/plugins/lvm/lvm-dbus.c
• src/plugins/nvdimm.c
• src/plugins/part.c
• tests/_lvm_cases.py

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Propagate the rename result after a successful convert.

Line 3718 still ignores bd_lvm_lvrename()'s return value. If CreateCachePool succeeds but the rename fails, bd_lvm_cache_pool_convert() can still return TRUE even though the caller asked for a different final LV name. The native path in src/plugins/lvm/lvm.c:2499-2500 already folds the rename result into the final status.

Suggested fix

-    if (ret && name)
-        bd_lvm_lvrename (vg_name, data_lv, name, NULL, error);
+    if (ret && name)
+        ret = bd_lvm_lvrename (vg_name, data_lv, name, NULL, error);

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if (ret && name)
	bd_lvm_lvrename (vg_name, data_lv, name, NULL, error);
	if (ret && name)
	ret = bd_lvm_lvrename (vg_name, data_lv, name, NULL, error);

🤖 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 3717 - 3718, The code ignores
bd_lvm_lvrename()'s return value causing bd_lvm_cache_pool_convert() to report
success even if the rename failed; update the call in bd_lvm_cache_pool_convert
so you capture and fold the rename result into the existing status (e.g., set
ret = ret && bd_lvm_lvrename(vg_name, data_lv, name, NULL, error)) so the final
return reflects both the CreateCachePool and the LV rename outcome.