Skip to content

fix: issues found via code review #170

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
richm merged 6 commits into from
May 7, 2026
Merged

fix: issues found via code review #170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
533fdc3
fix: remove trailing comma causing tuple instead of string assignment
trgill Apr 24, 2026
a18aa52
fix: correct return statement syntax in snapmgr check_mode paths
trgill Apr 24, 2026
bbe5a6e
fix: close file descriptor to prevent resource leak
trgill Apr 24, 2026
90d0ea7
fix: add null check for snapshot_lvm_snapset_name before endswith()
trgill Apr 24, 2026
cfe1062
fix: correct typos in user-facing error messages
trgill Apr 24, 2026
419755c
fix: remove redundant variable initialization flagged by CodeQL
trgill May 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions module_utils/snapshot_lsr/lvm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,9 @@ def get_json_from_args(module, module_args, vg_include):
vg_str = vg["vg_name"]
for lv in lv_list:

if lv["lv_name"].endswith(module_args["snapshot_lvm_snapset_name"]):
if module_args["snapshot_lvm_snapset_name"] and lv["lv_name"].endswith(
module_args["snapshot_lvm_snapset_name"]
):
logger.info(
"get_json_from_args: already a snapshot for %s", lv["lv_name"]
)
Expand Down Expand Up @@ -129,7 +131,7 @@ def get_json_from_args(module, module_args, vg_include):
)
continue
volume = {}
volume["name"] = ("snapshot : " + vg_str + "/" + lv["lv_name"],)
volume["name"] = "snapshot : " + vg_str + "/" + lv["lv_name"]
volume["vg"] = vg_str
volume["lv"] = lv["lv_name"]

Expand Down
26 changes: 16 additions & 10 deletions module_utils/snapshot_lsr/snapmgr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,11 @@ def mgr_snapshot_cmd(module, module_args, snapset_json):
source_list = mgr_get_source_list_for_create(volume_list)

if check_mode:
return {rc, "Would call function manager.create_snapshot_set()", False}
return {
"return_code": SnapshotStatus.SNAPSHOT_OK,
"errors": "Would call function manager.create_snapshot_set()",
"changed": False,
}

manager = snap_manager.Manager()

Expand Down Expand Up @@ -437,14 +441,15 @@ def mgr_extend_cmd(module, module_args, snapset_json):

if check_mode:
return {
rc,
"Would run function "
+ " manager.resize_snapshot_set() with ".join(source_list),
"return_code": rc,
"errors": "Would run function manager.resize_snapshot_set() with "
+ ", ".join(source_list),
"changed": False,
}

# there are no LVs that require an extend operation, return OK.
if len(source_list) == 0:
return {"return_code": rc, "errors": "source list emmpty", "changed": changed}
return {"return_code": rc, "errors": "source list empty", "changed": changed}

try:
manager.resize_snapshot_set(source_list, snapset_name)
Expand Down Expand Up @@ -472,9 +477,10 @@ def mgr_revert_cmd(module_args, snapset_json):

if check_mode:
return {
rc,
"Would run function "
+ " manager.revert_snapshot_set with ".join(snapset_name),
"return_code": rc,
"errors": "Would run function manager.revert_snapshot_set with "
+ snapset_name,
"changed": False,
}

manager = snap_manager.Manager()
Expand Down Expand Up @@ -512,7 +518,7 @@ def mgr_mount_cmd(module, module_args, snapset_json):
if snapshot_set is None or len(snapshot_set) == 0:
return {
"return_code": SnapshotStatus.ERROR_MOUNT_FAILED,
"errors": "snnapshot not found:" + snapset_name,
"errors": "snapshot not found: " + snapset_name,
"changed": False,
}

Expand Down Expand Up @@ -545,7 +551,7 @@ def mgr_umount_cmd(module, module_args, snapset_json):
if snapshot_set is None or len(snapshot_set) == 0:
return {
"return_code": SnapshotStatus.ERROR_MOUNT_FAILED,
"errors": "snnapshot not found:" + snapset_name,
"errors": "snapshot not found: " + snapset_name,
"changed": False,
}

Expand Down
6 changes: 4 additions & 2 deletions module_utils/snapshot_lsr/validate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ def get_json_from_args(module, module_args, vg_include):
vg_str = vg["vg_name"]
for lv in lv_list:

if lv["lv_name"].endswith(module_args["snapshot_lvm_snapset_name"]):
if module_args["snapshot_lvm_snapset_name"] and lv["lv_name"].endswith(
module_args["snapshot_lvm_snapset_name"]
):
logger.info(
"get_json_from_args: already a snapshot for %s", lv["lv_name"]
)
Expand Down Expand Up @@ -79,7 +81,7 @@ def get_json_from_args(module, module_args, vg_include):
)
continue
volume = {}
volume["name"] = ("snapshot : " + vg_str + "/" + lv["lv_name"],)
volume["name"] = "snapshot : " + vg_str + "/" + lv["lv_name"]
volume["vg"] = vg_str
volume["lv"] = lv["lv_name"]

Expand Down
3 changes: 2 additions & 1 deletion tests/library/find_unused_disk.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,8 @@ def no_holders(disk_path):
def can_open(disk_path):
"""Return true if the device can be opened with exclusive access."""
try:
os.open(disk_path, os.O_EXCL)
fd = os.open(disk_path, os.O_EXCL)
os.close(fd)
Comment on lines +120 to +121
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot May 5, 2026

Choose a reason for hiding this comment

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

suggestion (testing): Consider adding tests for can_open to verify file descriptor cleanup and error handling.

The fix removes the leak, but there are still no tests covering can_open. Please add tests that:

  1. Verify it returns True when os.open succeeds and that os.open/os.close are called as expected (no extra FDs left open).
  2. Verify it returns False when os.open raises OSError.

This will protect against regressions in both behavior and resource handling.

Suggested implementation:

def can_open(disk_path):
    """Return true if the device can be opened with exclusive access."""
    try:
        fd = os.open(disk_path, os.O_EXCL)
        os.close(fd)
        return True
    except OSError:
        return False


def test_can_open_returns_true_and_closes_fd(monkeypatch):
    """can_open returns True and closes the file descriptor when os.open succeeds."""
    calls = {"open": [], "close": []}

    def fake_open(path, flags):
        calls["open"].append((path, flags))
        return 42

    def fake_close(fd):
        calls["close"].append(fd)

    monkeypatch.setattr(os, "open", fake_open)
    monkeypatch.setattr(os, "close", fake_close)

    result = can_open("/dev/fake-disk")

    # Behavior: returns True on success
    assert result is True
    # Resource handling: exactly one open and one close with the correct FD
    assert calls["open"] == [("/dev/fake-disk", os.O_EXCL)]
    assert calls["close"] == [42]


def test_can_open_returns_false_on_oserror(monkeypatch):
    """can_open returns False and does not close any FD when os.open raises OSError."""
    def fake_open(path, flags):
        raise OSError("cannot open")

    close_calls = []

    def fake_close(fd):
        close_calls.append(fd)

    monkeypatch.setattr(os, "open", fake_open)
    monkeypatch.setattr(os, "close", fake_close)

    result = can_open("/dev/fake-disk")

    # Behavior: returns False on OSError
    assert result is False
    # Resource handling: os.close must not be called when open fails
    assert close_calls == []
  • Ensure os is imported at the top of tests/library/find_unused_disk.py (if it is not already): import os.
  • These tests assume pytest is used and that the monkeypatch fixture is available in your test configuration.

Copy link
Copy Markdown
Contributor

@richm richm May 6, 2026

Choose a reason for hiding this comment

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

@trgill yeah, seems like we should do the os.close here to avoid a potential fd leak

return True
except OSError:
return False
Expand Down
Loading