Check for non-existing/invalid/empty devices/paths in dm plugin and utils

[vojtechtrefny]

Summary by CodeRabbit

• Bug Fixes

  • Empty device-mapper name/node inputs now raise clear validation errors instead of proceeding.
  • Device resolution now verifies resolved paths are block devices and returns explicit errors for invalid targets.

• Tests

  • Expanded test coverage for device resolution and symlink handling using storage-backed test setups, including checks for non-block-device failures and symlink resolution behavior.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds input validation to device-mapper functions, introduces a block-device validation helper, switches several locals to g_autofree, defines a _C_LOCALE macro, and refactors tests to validate the new error behavior and use LVM/loop-device fixtures.

Changes

Cohort / File(s)	Summary
Device-mapper logic src/plugins/dm.c	Added null/empty input checks for DM name/node functions; replaced manual frees with g_autofree locals; adjusted control flow for early returns and preserved existing mapping/error paths.
Device utilities src/utils/dev_utils.c, src/utils/dev_utils.h	Added static _is_block_device() helper; integrated block-device validation into bd_utils_resolve_device(); switched temporary buffers to g_autofree; added _C_LOCALE macro in header.
Tests — dm API tests/dm_test.py	Updated tests to assert GLib.GError is raised for empty DM name/node inputs with specific error messages.
Tests — device resolution & LVM fixtures tests/utils_test.py	Refactored test classes to UtilsStorageTestCase with loop-device and LVM setup/teardown; updated/restructured tests to use dynamic loop + LVM paths and added assertions for non-block-device errors.

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 66.67% 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 objective: adding validation checks for non-existing, invalid, or empty devices/paths in the dm plugin and utils components.

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[vojtechtrefny]

New private symbols found:
_is_block_device

Good to see the test is actually working. (Of course I definitely meant to test this, I wouldn't forget to add the static keyword like that.)

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/utils/dev_utils.h (1)

20-29: ⚠️ Potential issue | 🟠 Major

Add <locale.h> for locale_t and rename _C_LOCALE to avoid reserved identifier.

Line 29 introduces locale_t in a public header without including <locale.h>. While compilation currently succeeds because consuming .c files include <locale.h> themselves, this violates header self-containment and can break code that includes this header in isolation. Additionally, _C_LOCALE uses a reserved identifier pattern (underscore followed by uppercase letter); per the C standard, such names are reserved for the implementation. Rename to BD_UTILS_C_LOCALE or move the macro to a private header if it need not be public.

🔧 Proposed fix

 `#include` <glib.h>
+#include <locale.h>
 
 `#ifndef` BD_UTILS_DEV_UTILS
 `#define` BD_UTILS_DEV_UTILS
 
 GQuark bd_utils_dev_utils_error_quark (void);
 `#define` BD_UTILS_DEV_UTILS_ERROR bd_utils_dev_utils_error_quark ()
 
 /* "C" locale to get the locale-agnostic error messages */
-#define _C_LOCALE (locale_t) 0
+#define BD_UTILS_C_LOCALE (locale_t) 0

🤖 Fix all issues with AI agents

In `@src/plugins/dm.c`:
- Around line 215-223: In bd_dm_node_from_name, validate map_name (check for
NULL or empty) before using it to build dev_mapper_path; either move the
existing guard block above the g_strdup_printf call or initialize
dev_mapper_path to NULL and assign it after the guard, ensuring dev_mapper_path
is only created with a non-NULL map_name to avoid passing NULL into
g_strdup_printf (refer to variables map_name and dev_mapper_path and function
bd_dm_node_from_name).
- Around line 178-187: In bd_dm_name_from_node validate dm_node (check for NULL
or empty string) before calling g_strdup_printf so you never pass a NULL to
g_strdup_printf; move the existing if (!dm_node || strlen(dm_node) == 0) check
to the top of the function and only then build sys_path with
g_strdup_printf("/sys/class/block/%s/dm/name", dm_node), keeping the rest of the
error handling (g_set_error) and return semantics unchanged.

In `@tests/utils_test.py`:
- Around line 409-438: In setUp, the RuntimeError rethrown after
create_lio_device loses the original exception context; modify the except block
in UtilsStorageTestCase.setUp to re-raise the new RuntimeError using exception
chaining (raise RuntimeError("Failed to setup loop device for testing: %s" % e)
from e) so the original traceback from create_lio_device is preserved.