virttest/staging/lv_utils: ignore warnings

[smitterl]

The current implementation filters the data through grep. However, in some cases a warning message might be displayed, such as

"""
File descriptor 3 (/dev/z90crypt) leaked on pvs invocation. Parent PID 5118: -bash """

While this is a valuable piece of information, the test case shouldn't break on it if the command succeeds and the necessary information is found in the output; the function focuses on identifying the blk target.

So, instead, read the pvs output and identify error conditions (command failure, empty output). If these are not given, identify the relevant parts as before from those output lines, that is the code does the filtering, not grep.

Summary by CodeRabbit

• Bug Fixes
  • Improved reliability when retrieving volume-group mappings by handling command failures, empty responses, and varied output formatting to avoid errors.
  • More robust parsing via line-by-line scanning, skipping headers/non-data lines and validating entries to prevent incorrect matches and parsing failures.
• Chores
  • Updated spell-ignore list with two additional tokens to reduce false positives in spell checks.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The function get_vg_mapped_blk_target in virttest/staging/lv_utils.py now imports utils_misc and uses utils_misc.cmd_status_output(cmd, shell=True, session=session) to run pvs. It checks the command status and empty output, returns None on failure, skips the header, and parses the pvs output line-by-line to find the row for the target vg_name. When found it extracts the PV name (first column) and derives the block target by taking the alphabetic prefix after /dev/, guarded by try/except. Returns the derived block target or None.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Review correct use and parameters of utils_misc.cmd_status_output(...).
• Validate header-skipping and line-splitting logic against real pvs output formats (multi-space, tabs, empty lines).
• Confirm PV name extraction and the alphabetic-prefix derivation after /dev/ handle edge cases.
• Check error paths: non-zero status, empty output, and exception handling in parsing.

Pre-merge checks

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title 'virttest/staging/lv_utils: ignore warnings' is partially related to the changeset but does not accurately reflect the main change, which is refactoring get_vg_mapped_blk_target to parse pvs output directly instead of using grep, making it robust to warning lines. The title suggests only adding warning ignores but misses the core refactoring.	Consider a more descriptive title like 'virttest/staging/lv_utils: refactor get_vg_mapped_blk_target to handle pvs warnings' to better capture the main change of switching from grep-based filtering to direct output parsing.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions:
   | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context.
   Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

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: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2defbcc and ff3db62.

📒 Files selected for processing (1)

• virttest/staging/lv_utils.py (2 hunks)

🧰 Additional context used

🪛 GitHub Actions: CI

virttest/staging/lv_utils.py

[error] 463-463: Black formatting would reformat /home/runner/work/avocado-vt/avocado-vt/virttest/staging/lv_utils.py. Run 'black' to format the file.

🪛 Ruff (0.14.5)

virttest/staging/lv_utils.py

453-453: Function call with shell=True parameter identified, security issue

(S604)

🔇 Additional comments (2)

virttest/staging/lv_utils.py (2)

51-51: LGTM!

The import is necessary for the refactored get_vg_mapped_blk_target function.

---

452-453: Shell usage is safe here.

The static analysis warning about shell=True is a false positive in this case, as the command is hardcoded with no user-controlled input.

[smitterl]

The following test case failed due to the given description with "ERROR: list index out of range" but passes now with this change:

JOB ID     : 9a7168a4531f6850a32804a541af3a552d68d4e2
JOB LOG    : /var/log/avocado/job-results/job-2025-11-24T11.37-9a7168a/job.log
 (1/1) type_specific.io-github-autotest-libvirt.virsh.domfsinfo.positive_tests.no_freezed_fs: STARTED
 (1/1) type_specific.io-github-autotest-libvirt.virsh.domfsinfo.positive_tests.no_freezed_fs: PASS (25.49 s)
RESULTS    : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB HTML   : /var/log/avocado/job-results/job-2025-11-24T11.37-9a7168a/results.html
JOB TIME   : 26.93 s

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

virttest/staging/lv_utils.py (1)

451-477: Tighten VG matching and device-name regex to avoid incorrect blk target resolution.

The overall restructuring (status check, empty-output handling, line‑by‑line parsing) is a solid improvement, but two details can still cause subtle bugs:

• if vg_name in line: can match unintended VGs (e.g. searching for rhel will also match rhel-test).
• re.findall(r"[a-z]+", ...) only matches lowercase letters and may fail or misbehave for mixed/uppercase device names.

You can make this more robust by matching the VG column exactly and broadening the regex, e.g.:

-    for line in lines[1:]:
-        if vg_name in line:
-            parts = line.split()
-            if parts:
-                pv_name = parts[0]
-                try:
-                    blk_target = re.findall(r"[a-z]+", pv_name.split("/dev/")[1])[0]
-                    break
-                except (IndexError, AttributeError):
-                    continue
+    for line in lines[1:]:
+        parts = line.split()
+        # Expect columns like: PV VG Fmt Attr PSize PFree
+        if len(parts) >= 2 and parts[1] == vg_name:
+            pv_name = parts[0]
+            try:
+                blk_target = re.findall(r"[a-zA-Z]+", pv_name.split("/dev/")[1])[0]
+                break
+            except (IndexError, AttributeError):
+                continue

This keeps the new parsing approach but avoids false positives and case-related issues when resolving the block target.

🧹 Nitpick comments (1)

virttest/staging/lv_utils.py (1)

451-453: Ruff S604 on shell=True: safe here but worth documenting/guarding.

utils_misc.cmd_status_output(cmd, shell=True, session=session) is invoked with a constant cmd = "pvs", so command injection risk is effectively nil, but Ruff still reports S604 for shell=True.

If you expect cmd to stay a fixed literal, I'd either:

• Keep this as‑is and add a short comment explaining that cmd is constant (or annotate with the project’s preferred “ignore S604” mechanism), or
• Refactor later to pass an argument list with shell=False if utils_misc supports it.

This is low priority but will help future readers and static analysis if cmd ever becomes parameterized.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 488e04d and 7d61c4d.

📒 Files selected for processing (2)

• spell.ignore (1 hunks)
• virttest/staging/lv_utils.py (2 hunks)

🧰 Additional context used

🪛 Ruff (0.14.6)

virttest/staging/lv_utils.py

452-452: Function call with shell=True parameter identified, security issue

(S604)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Static checks

🔇 Additional comments (2)

spell.ignore (1)

59-60: LVM column names correctly added to spell-ignore list.

Adding PFree and PSize here matches pvs column headers and avoids false positives in spell checking; no further changes needed.

virttest/staging/lv_utils.py (1)

50-50: Import of utils_misc is appropriate and required.

The added utils_misc import is correctly scoped and used by get_vg_mapped_blk_target; no issues here.