fix: issues found via code review#170
Merged
Merged
Conversation
Fixed volume["name"] assignments in lvm.py and validate.py where a
trailing comma was creating a tuple ('snapshot : vg/lv',) instead of
a string "snapshot : vg/lv".
Signed-off-by: Todd Gill <tgill@redhat.com>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideFixes several correctness and robustness issues in the snapshot role, including data type bugs, check-mode return value structure, resource cleanup, null safety, and typo corrections, while preserving external behavior. Flow diagram for get_json_from_args null-safe snapshot detectionflowchart TD
A[Start get_json_from_args] --> B[Iterate_volume_groups]
B --> C[Iterate_logical_volumes]
C --> D{snapshot_lvm_snapset_name_not_none}
D -->|No| H[Skip_snapshot_name_suffix_check]
D -->|Yes| E{lv_name_endswith_snapshot_lvm_snapset_name}
E -->|Yes| F[Log_already_a_snapshot_for_lv]
F --> C
E -->|No| G[Validate_lv_and_vg]
G --> I{Validation_passed}
I -->|No| J[Log_and_continue_to_next_lv]
J --> C
I -->|Yes| K[Create_volume_dict]
K --> L[Set_volume_name_as_string_snapshot_colon_vg_slash_lv]
L --> M[Set_volume_vg]
M --> N[Set_volume_lv]
N --> C
H --> G
C --> O[End_when_all_lvs_processed]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In the updated
.endswith()guards, consider usingmodule_args.get("snapshot_lvm_snapset_name") is not None(or similar) instead of a generic truthiness check so that an empty string doesn’t silently skip matches and to avoidKeyErrorif the key is missing. - In the check-mode return paths (e.g.,
mgr_snapshot_cmd,mgr_extend_cmd,mgr_revert_cmd), you’re using the"errors"field to carry informational messages; if these are not true errors, you may want to introduce a separate field (like"message") to distinguish normal check-mode output from actual error conditions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the updated `.endswith()` guards, consider using `module_args.get("snapshot_lvm_snapset_name") is not None` (or similar) instead of a generic truthiness check so that an empty string doesn’t silently skip matches and to avoid `KeyError` if the key is missing.
- In the check-mode return paths (e.g., `mgr_snapshot_cmd`, `mgr_extend_cmd`, `mgr_revert_cmd`), you’re using the `"errors"` field to carry informational messages; if these are not true errors, you may want to introduce a separate field (like `"message"`) to distinguish normal check-mode output from actual error conditions.
## Individual Comments
### Comment 1
<location path="module_utils/snapshot_lsr/lvm.py" line_range="99" />
<code_context>
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"]
</code_context>
<issue_to_address>
**question (bug_risk):** Guarding by truthiness may change behavior when `snapshot_lvm_snapset_name` is an empty string
Previously, an empty `snapshot_lvm_snapset_name` caused `lv["lv_name"].endswith(...)` to match all LVs; now the condition is skipped in that case. If an empty string is supposed to mean “match everything,” this changes behavior. If you only want to check when a non-empty name is given, this is fine; otherwise, guard explicitly (e.g., `is not None`) or use a sentinel default to distinguish “no filter” from “empty string.”
</issue_to_address>
### Comment 2
<location path="module_utils/snapshot_lsr/validate.py" line_range="49" />
<code_context>
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"]
</code_context>
<issue_to_address>
**question:** Validate that the new truthiness guard matches the intended semantics for `snapshot_lvm_snapset_name`
Because this now short-circuits on generic truthiness, any falsy value (like an empty string) will skip the `endswith` check. Please confirm that’s the desired behavior for “unset” names; if not, narrow the guard to only the specific values you consider as not configured (e.g. explicitly checking for `None`).
</issue_to_address>
### Comment 3
<location path="tests/library/find_unused_disk.py" line_range="120-121" />
<code_context>
"""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)
return True
except OSError:
</code_context>
<issue_to_address>
**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:
```python
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.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| 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"]): |
There was a problem hiding this comment.
question (bug_risk): Guarding by truthiness may change behavior when snapshot_lvm_snapset_name is an empty string
Previously, an empty snapshot_lvm_snapset_name caused lv["lv_name"].endswith(...) to match all LVs; now the condition is skipped in that case. If an empty string is supposed to mean “match everything,” this changes behavior. If you only want to check when a non-empty name is given, this is fine; otherwise, guard explicitly (e.g., is not None) or use a sentinel default to distinguish “no filter” from “empty string.”
| 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"]): |
There was a problem hiding this comment.
question: Validate that the new truthiness guard matches the intended semantics for snapshot_lvm_snapset_name
Because this now short-circuits on generic truthiness, any falsy value (like an empty string) will skip the endswith check. Please confirm that’s the desired behavior for “unset” names; if not, narrow the guard to only the specific values you consider as not configured (e.g. explicitly checking for None).
Comment on lines
+120
to
+121
| fd = os.open(disk_path, os.O_EXCL) | ||
| os.close(fd) |
There was a problem hiding this comment.
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:
- Verify it returns
Truewhenos.opensucceeds and thatos.open/os.closeare called as expected (no extra FDs left open). - Verify it returns
Falsewhenos.openraisesOSError.
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
osis imported at the top oftests/library/find_unused_disk.py(if it is not already):import os. - These tests assume pytest is used and that the
monkeypatchfixture is available in your test configuration.
Contributor
There was a problem hiding this comment.
@trgill yeah, seems like we should do the os.close here to avoid a potential fd leak
Fixed three locations where return statements were creating sets instead
of dicts due to missing key-value syntax. All check_mode returns now
properly return {"return_code": rc, "errors": message, "changed": False}.
Also initialized rc variable at the start of mgr_snapshot_cmd to ensure
it's defined for all code paths.
Signed-off-by: Todd Gill <tgill@redhat.com>
The can_open() function was opening file descriptors with os.open() but never closing them, causing a resource leak. Now properly closes the file descriptor after verifying exclusive access is possible. Signed-off-by: Todd Gill <tgill@redhat.com>
Added null checks in lvm.py and validate.py to prevent AttributeError when snapshot_lvm_snapset_name is None. The code now verifies the value exists before calling .endswith() on it. Signed-off-by: Todd Gill <tgill@redhat.com>
Fixed three typos in snapmgr.py error messages: - "emmpty" → "empty" - "snnapshot" → "snapshot" (2 occurrences) Also improved formatting by adding space after colon in error message. Signed-off-by: Todd Gill <tgill@redhat.com>
trgill
changed the title
spetrosi
approved these changes
May 6, 2026
Removed dead store of rc variable in mgr_snapshot_cmd(). The variable was initialized to SnapshotStatus.SNAPSHOT_OK but immediately overwritten by verify_snapset_source_lvs_exist() before ever being used. In the check_mode path, explicitly use SnapshotStatus.SNAPSHOT_OK instead of the rc variable for clarity, since we know rc must be SNAPSHOT_OK at that point (or we would have returned early on line 213-214). This fixes CodeQL alert py/multiple-definition: 'This assignment to rc is unnecessary as it is redefined before this value is used.' Addresses: https://github.com/linux-system-roles/snapshot/security/code-scanning/95 Signed-off-by: Todd Gill <tgill@redhat.com>
Collaborator
Author
|
[citest] |
richm
approved these changes
May 7, 2026
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
4 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.
Enhancement:
This PR addresses issues in the snapshot role. The fixes include:
can_open()function.endswith()on potentially None valuesReason:
Code review, issues identified:
('snapshot : vg/lv',)instead of strings, which could cause unexpected behavior in string operations and serialization{0, "message", False}instead of properly formatted dictionaries, causing the module to failfind_unused_diskmodule was leaking file descriptors during disk scanning operations, potentially exhausting system resources on systems with many disksAttributeErrorexceptions when optional parameters are not providedResult:
All fixes maintain backward compatibility and do not change the external API or behavior of the role.
Issue Tracker Tickets (Jira or BZ if any):
Summary by Sourcery
Fix snapshot management role issues uncovered in review, correcting return structures, data types, error messages, and resource handling.
Bug Fixes:
Enhancements: